[{"id":26485,"web_url":"https://patchwork.libcamera.org/comment/26485/","msgid":"","date":"2023-02-27T16:52:51","subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Mattijs,\n\nOn Mon, Feb 27, 2023 at 09:54:02AM +0100, Mattijs Korpershoek via libcamera-devel wrote:\n> PlatformFrameBufferAllocator is an abstraction over gralloc.\n> Right now hardwareModule_ points towards a CAMERA_HARDWARE_MODULE_ID.\n> \n> When gralloc_open() is called we observe:\n> libcamera: DEBUG HAL camera3_hal.cpp:75 Open camera gpu0\n> libcamera: ERROR Camera camera.cpp:524 Camera in Configured state trying acquire() requiring state Available\n> 01-23 14:14:04.742 370 416 E libcamera: FATAL HAL generic_frame_buffer_allocator.cpp:105 gralloc_open() failed: -87\n> \n> Which is wrong, gralloc_open() is attempting to re-open the camera HAL,\n> instead of the gralloc HAL.\n> \n> Point to a GRALLOC_HARDWARE_MODULE_ID instead so that we can request\n> buffers from gralloc in android.\n\nGood catch !\n\n> Note: this add a new dependency on android's libhardware [1]\n> \n> [1] https://android.googlesource.com/platform/hardware/libhardware\n> \n> Fixes: c58662c5770e (\"android: Introduce PlatformFrameBufferAllocator\")\n> Signed-off-by: Mattijs Korpershoek \n> ---\n> RFC as this adds an additional link-time dependency on an AOSP project\n> (libhardware) and I'm unsure how to handle this.\n> \n> I thought of using the subprojects structure as done for libyuv but\n> since libhardware has other AOSP deps (like libvnksupport) I'm not sure\n> this is appropriate.\n> \n> Any thoughts on this?\n\nWe can require linking against AOSP libraries when compiling this code,\nthat's not an issue. A subproject isn't needed (and wouldn't be\npractical anyway).\n\nHow did you test-link this patch ?\n\n> ---\n> src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> index 3750e1bf52a9..58e6c68c4998 100644\n> --- a/src/android/mm/generic_frame_buffer_allocator.cpp\n> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> @@ -72,9 +72,10 @@ class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> public:\n> \tPrivate(CameraDevice *const cameraDevice)\n> \t\t: cameraDevice_(cameraDevice),\n> -\t\t hardwareModule_(cameraDevice->camera3Device()->common.module),\n> +\t\t hardwareModule_(nullptr),\n> \t\t allocDevice_(nullptr)\n> \t{\n> +\t\thw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hardwareModule_);\n\nhw_get_module(), if my understanding is correct, ends up dlopen()ing the\nmodule. Is there anything we need to do in the destructor to close the\nmodule ?\n\n> \t\tASSERT(hardwareModule_);\n> \t}\n> \n> @@ -85,7 +86,7 @@ public:\n> \n> private:\n> \tconst CameraDevice *const cameraDevice_;\n> -\tstruct hw_module_t *const hardwareModule_;\n> +\tconst struct hw_module_t *hardwareModule_;\n> \tstruct alloc_device_t *allocDevice_;\n> };\n> \n> \n> ---\n> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca\n> change-id: 20230227-android-gralloc-04e31d5edc68","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 78C37BE08A\n\tfor ;\n\tMon, 27 Feb 2023 16:52:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBBE76267E;\n\tMon, 27 Feb 2023 17:52:56 +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 34E5A62636\n\tfor ;\n\tMon, 27 Feb 2023 17:52:55 +0100 (CET)","from pendragon.ideasonboard.com (85-76-46-30-nat.elisa-mobile.fi\n\t[85.76.46.30])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9214FB6C;\n\tMon, 27 Feb 2023 17:52:53 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677516776;\n\tbh=/Vcn8lZesigmQdVMD23zwosO2Ku1Y2ZCExqBf/lHSd8=;\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=zvlockAmVVktKRFQAzS+AnZ0f7rLxJWeyCQhiLDvpj6l/QSPQOAvBqHkVR3OWsMe7\n\tkDrO17X+DvC34OUtyREhqO2UEYBc/wkxYOApcnZW2gGqdDWkL/UXsnyrzZsB98oS60\n\tYeWdHdRv9IXIhPmsi9ITTPszbkrTIUWErNWus0l8B8QPVDMF3KIOlrCXj/8WrLKKJ9\n\te6kFsgIQ4YcWwCutWjhPma8iONdQ1oPxbFW+C/MuAAn2wLFxcjxtT4V+VIBUDDS3a+\n\t9pSYwrGWZPy9yCXVv5cEA/FvbWO9OPV6g5mguJhTylD1ojVDFcOtTkX90eNsDsiS4C\n\tE9jfKXN5f1CtQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677516774;\n\tbh=/Vcn8lZesigmQdVMD23zwosO2Ku1Y2ZCExqBf/lHSd8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NNPJdZ0w04+KAyz+BrM/h/41G4xAGCPjM0zL0IWSUuD1Ng+7mo2TLGEnL7YoEpifd\n\takKtM8NU1qou9qvgWuCxLqqZ/tADYnyslqDPjUA/er6d/A+KwFyw/fCknHWMbPhWQB\n\tvZatTvia7ivCKjpRNNKKwn8BHBaBf2bDJ9I5QUps="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NNPJdZ0w\"; dkim-atps=neutral","Date":"Mon, 27 Feb 2023 18:52:51 +0200","To":"Mattijs Korpershoek ","Message-ID":"","References":"<20230227-android-gralloc-v1-1-3360fcfa7c0d@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230227-android-gralloc-v1-1-3360fcfa7c0d@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","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":26496,"web_url":"https://patchwork.libcamera.org/comment/26496/","msgid":"<871qm9lx20.fsf@baylibre.com>","date":"2023-02-28T13:49:27","subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","submitter":{"id":153,"url":"https://patchwork.libcamera.org/api/people/153/","name":"Mattijs Korpershoek","email":"mkorpershoek@baylibre.com"},"content":"Hi Laurent,\n\nThank you for your review.\n\nOn lun., févr. 27, 2023 at 18:52, Laurent Pinchart wrote:\n\n> Hi Mattijs,\n>\n> On Mon, Feb 27, 2023 at 09:54:02AM +0100, Mattijs Korpershoek via libcamera-devel wrote:\n>> PlatformFrameBufferAllocator is an abstraction over gralloc.\n>> Right now hardwareModule_ points towards a CAMERA_HARDWARE_MODULE_ID.\n>> \n>> When gralloc_open() is called we observe:\n>> libcamera: DEBUG HAL camera3_hal.cpp:75 Open camera gpu0\n>> libcamera: ERROR Camera camera.cpp:524 Camera in Configured state trying acquire() requiring state Available\n>> 01-23 14:14:04.742 370 416 E libcamera: FATAL HAL generic_frame_buffer_allocator.cpp:105 gralloc_open() failed: -87\n>> \n>> Which is wrong, gralloc_open() is attempting to re-open the camera HAL,\n>> instead of the gralloc HAL.\n>> \n>> Point to a GRALLOC_HARDWARE_MODULE_ID instead so that we can request\n>> buffers from gralloc in android.\n>\n> Good catch !\n>\n>> Note: this add a new dependency on android's libhardware [1]\n>> \n>> [1] https://android.googlesource.com/platform/hardware/libhardware\n>> \n>> Fixes: c58662c5770e (\"android: Introduce PlatformFrameBufferAllocator\")\n>> Signed-off-by: Mattijs Korpershoek \n>> ---\n>> RFC as this adds an additional link-time dependency on an AOSP project\n>> (libhardware) and I'm unsure how to handle this.\n>> \n>> I thought of using the subprojects structure as done for libyuv but\n>> since libhardware has other AOSP deps (like libvnksupport) I'm not sure\n>> this is appropriate.\n>> \n>> Any thoughts on this?\n>\n> We can require linking against AOSP libraries when compiling this code,\n> that's not an issue. A subproject isn't needed (and wouldn't be\n> practical anyway).\n\nThank you, that is helpful.\n\nDoes that mean that the change can be kept as-is, and it's acceptable to\nhave link errors when building with\n-Dandroid=enabled -Dandroid_platform=generic ?\n\n>\n> How did you test-link this patch ?\n\nI have link-tested this patch in an Android 12 AOSP-based environment.\n\nI also build-tested using the meson build system, noticing the link error.\n\nI work in a BayLibre specific integration, where have generated\nAndroid.bp files with a python script based on the meson files. It\nparses the meson.build files to generate Android.bp files.\n\nWe use this to build libcamera, the Android Camera HAL and the cam app.\n\nAs I'm not sure sure the libcamera wants to maintain this, I did not\nsubmit it.\nIf this seems useful for libcamera, I can polish this up and submit it.\n\n>\n>> ---\n>> src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++--\n>> 1 file changed, 3 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n>> index 3750e1bf52a9..58e6c68c4998 100644\n>> --- a/src/android/mm/generic_frame_buffer_allocator.cpp\n>> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n>> @@ -72,9 +72,10 @@ class PlatformFrameBufferAllocator::Private : public Extensible::Private\n>> public:\n>> \tPrivate(CameraDevice *const cameraDevice)\n>> \t\t: cameraDevice_(cameraDevice),\n>> -\t\t hardwareModule_(cameraDevice->camera3Device()->common.module),\n>> +\t\t hardwareModule_(nullptr),\n>> \t\t allocDevice_(nullptr)\n>> \t{\n>> +\t\thw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hardwareModule_);\n>\n> hw_get_module(), if my understanding is correct, ends up dlopen()ing the\n> module. Is there anything we need to do in the destructor to close the\n> module ?\n\nYour understanding is correct, hw_get_module() ends up doing dlopen().\nAFAIK, there is no equivalent of release the hardwareModule_. There is\nno hw_put_module().\n\nI've skimmed through some other implementations of HALs (such as\ngatekeeper) and they indeed call dclose().\n\nThanks a lot for bringing this up. I will send a v2.\n\n\n>\n>> \t\tASSERT(hardwareModule_);\n>> \t}\n>> \n>> @@ -85,7 +86,7 @@ public:\n>> \n>> private:\n>> \tconst CameraDevice *const cameraDevice_;\n>> -\tstruct hw_module_t *const hardwareModule_;\n>> +\tconst struct hw_module_t *hardwareModule_;\n>> \tstruct alloc_device_t *allocDevice_;\n>> };\n>> \n>> \n>> ---\n>> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca\n>> change-id: 20230227-android-gralloc-04e31d5edc68\n>\n> -- \n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 24854BE080\n\tfor ;\n\tTue, 28 Feb 2023 13:49:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 87B3B62691;\n\tTue, 28 Feb 2023 14:49:31 +0100 (CET)","from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com\n\t[IPv6:2a00:1450:4864:20::42c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4BFEE62671\n\tfor ;\n\tTue, 28 Feb 2023 14:49:29 +0100 (CET)","by mail-wr1-x42c.google.com with SMTP id bx12so6659059wrb.11\n\tfor ;\n\tTue, 28 Feb 2023 05:49:29 -0800 (PST)","from localhost ([82.66.159.240]) by smtp.gmail.com with ESMTPSA id\n\tc10-20020a5d4f0a000000b002c54911f50bsm9904280wru.84.2023.02.28.05.49.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 28 Feb 2023 05:49:28 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677592171;\n\tbh=kTkz5yCov4JfreDAbTRRbooc5aPeqbLPjOdEVIa54ug=;\n\th=To:In-Reply-To:References:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rjudXYK6SVD6ukqa7++o6WCf+2e9O4TB32Pcq415aoSrIp3jItwiSVK4E2/ZJIS2B\n\t4WQclvM14jgfgZZb6EsIK4gAIg0AbeOSB5PeI2YFKiDdx7LjxB6FIGtjPwwAcq001b\n\tPBQCoHv2CartvKZDMGYufsKHz5Bw2oj53YHOFRnw4/NY1B2qQoUvY2i4n3po8krIZS\n\tArmXtAMSx6l+SxOZu0/cdYTV+FULZRLS/4onvrMsKGaWZaQnf7R5I0X7fc7qf5bhHa\n\tFWn7mxJmm8BDl0eMLaysFKCXN0Ja6dgHLgwqCV9iW/fqTdOkHs1tEvkjgkebXVrP5p\n\t3mVKEzBKXfFKA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112;\n\th=content-transfer-encoding:mime-version:message-id:date:references\n\t:in-reply-to:subject:cc:to:from:from:to:cc:subject:date:message-id\n\t:reply-to; bh=P6jwRboyabxXv8GhMdfBXMqom3ku8k1OWmXavb9siKs=;\n\tb=UMl0T2OosehsKQae3Ftq7rVa+ZJpIvRC6zXiWK/I/yprXGCe5deKr563Ds5Nt1EPwR\n\tIVvot4Iss5i5r0DFbH9HjKb+SgGo+yrhr/K6S2aJc6UrZLqxMQPlcG/Y7yUt2vhM0BDL\n\tsW9KhFl+qwugKRbZmUaVLKpAYvD190Si9URqHMKz9eOz732bmNYNB3HHMvm2EXiZ1HZe\n\t4WwUkSVLs3Vnff06mgXzw2t+kQSyIcDJBK6G0nh0Df4mY1gmlp9OYGCd0ZfqgmtNODtp\n\tpeKUtjIz8tScOmeHtayTQAgrMiCJX352XMx5cMQXdk+MJyB7jCFFJLEpbTH8SneTcwIc\n\tLSDQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"UMl0T2Oo\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=content-transfer-encoding:mime-version:message-id:date:references\n\t:in-reply-to:subject:cc:to:from:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=P6jwRboyabxXv8GhMdfBXMqom3ku8k1OWmXavb9siKs=;\n\tb=O3ltVOrgPSKmeW2TN+URlKS8DKm7tBtLPlfEcpEAD85r9LYm1+7avX/6R0b2ab2wJy\n\tKLJtmx8CuhzUaYoiO/8pEeYtzJT3YhPptJ0goasyy4XkZh91I+pqmAWBM5ZZVvwy3UIf\n\tHVV+SAy4KWV/kZIJ6VaqnmI7ZaNTgK4UfFIlC7QdJFJz7ScguL3rbdkO65OK0bt0DJOx\n\t8VcN0iUA/YrslgzVDSP1yonnwgbmTM/Mwu6CexOnYpfl9AClqtKOeOfqSz0K0+BptbzS\n\t78UV4X1gBjyBT4GLGvFhU+pIIrwSOfH3Pecww3tPfm7jwE/dbEJ433D7mUdWSA5+Lzbe\n\tVMTA==","X-Gm-Message-State":"AO0yUKV9BFzAO1ZbD3olFzGgj5J94zn6X0KvBEMLOe6R2CqdDm4xBinv\n\tIAQzIi96ZudzG0I5kJoSu65klA==","X-Google-Smtp-Source":"AK7set87ursECwknUgWN91VbH2nLgK9ifNX3ONQIBhKGk0qrm8xCGqy2g3zVst2wxZBx1vGmjt60+g==","X-Received":"by 2002:a5d:658f:0:b0:2c5:5391:8ab1 with SMTP id\n\tq15-20020a5d658f000000b002c553918ab1mr1988978wru.53.1677592168731; \n\tTue, 28 Feb 2023 05:49:28 -0800 (PST)","To":"Laurent Pinchart ","In-Reply-To":"","References":"<20230227-android-gralloc-v1-1-3360fcfa7c0d@baylibre.com>\n\t","Date":"Tue, 28 Feb 2023 14:49:27 +0100","Message-ID":"<871qm9lx20.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","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":"Mattijs Korpershoek via libcamera-devel\n\t","Reply-To":"Mattijs Korpershoek ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26503,"web_url":"https://patchwork.libcamera.org/comment/26503/","msgid":"","date":"2023-02-28T23:20:13","subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Mattijs,\n\nOn Tue, Feb 28, 2023 at 02:49:27PM +0100, Mattijs Korpershoek wrote:\n> Hi Laurent,\n> \n> Thank you for your review.\n> \n> On lun., févr. 27, 2023 at 18:52, Laurent Pinchart wrote:\n> \n> > Hi Mattijs,\n> >\n> > On Mon, Feb 27, 2023 at 09:54:02AM +0100, Mattijs Korpershoek via libcamera-devel wrote:\n> >> PlatformFrameBufferAllocator is an abstraction over gralloc.\n> >> Right now hardwareModule_ points towards a CAMERA_HARDWARE_MODULE_ID.\n> >> \n> >> When gralloc_open() is called we observe:\n> >> libcamera: DEBUG HAL camera3_hal.cpp:75 Open camera gpu0\n> >> libcamera: ERROR Camera camera.cpp:524 Camera in Configured state trying acquire() requiring state Available\n> >> 01-23 14:14:04.742 370 416 E libcamera: FATAL HAL generic_frame_buffer_allocator.cpp:105 gralloc_open() failed: -87\n> >> \n> >> Which is wrong, gralloc_open() is attempting to re-open the camera HAL,\n> >> instead of the gralloc HAL.\n> >> \n> >> Point to a GRALLOC_HARDWARE_MODULE_ID instead so that we can request\n> >> buffers from gralloc in android.\n> >\n> > Good catch !\n> >\n> >> Note: this add a new dependency on android's libhardware [1]\n> >> \n> >> [1] https://android.googlesource.com/platform/hardware/libhardware\n> >> \n> >> Fixes: c58662c5770e (\"android: Introduce PlatformFrameBufferAllocator\")\n> >> Signed-off-by: Mattijs Korpershoek \n> >> ---\n> >> RFC as this adds an additional link-time dependency on an AOSP project\n> >> (libhardware) and I'm unsure how to handle this.\n> >> \n> >> I thought of using the subprojects structure as done for libyuv but\n> >> since libhardware has other AOSP deps (like libvnksupport) I'm not sure\n> >> this is appropriate.\n> >> \n> >> Any thoughts on this?\n> >\n> > We can require linking against AOSP libraries when compiling this code,\n> > that's not an issue. A subproject isn't needed (and wouldn't be\n> > practical anyway).\n> \n> Thank you, that is helpful.\n> \n> Does that mean that the change can be kept as-is, and it's acceptable to\n> have link errors when building with\n> -Dandroid=enabled -Dandroid_platform=generic ?\n\nIt should fail at meson setup time, when creating the libhardware\ndependency.\n\n> > How did you test-link this patch ?\n> \n> I have link-tested this patch in an Android 12 AOSP-based environment.\n> \n> I also build-tested using the meson build system, noticing the link error.\n> \n> I work in a BayLibre specific integration, where have generated\n> Android.bp files with a python script based on the meson files. It\n> parses the meson.build files to generate Android.bp files.\n> \n> We use this to build libcamera, the Android Camera HAL and the cam app.\n> \n> As I'm not sure sure the libcamera wants to maintain this, I did not\n> submit it.\n\nFor now, our plan is to support building against the NDK, but a script\nthat generates Android.bp files is something that could be interesting.\nWe wouldn't commit the generated files to the repository, but we could\nhost the script until a better option is found. I wonder if it could be\nimplemented as a meson backend...\n\n> If this seems useful for libcamera, I can polish this up and submit it.\n> \n> >> ---\n> >> src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++--\n> >> 1 file changed, 3 insertions(+), 2 deletions(-)\n> >> \n> >> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> >> index 3750e1bf52a9..58e6c68c4998 100644\n> >> --- a/src/android/mm/generic_frame_buffer_allocator.cpp\n> >> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> >> @@ -72,9 +72,10 @@ class PlatformFrameBufferAllocator::Private : public Extensible::Private\n> >> public:\n> >> \tPrivate(CameraDevice *const cameraDevice)\n> >> \t\t: cameraDevice_(cameraDevice),\n> >> -\t\t hardwareModule_(cameraDevice->camera3Device()->common.module),\n> >> +\t\t hardwareModule_(nullptr),\n> >> \t\t allocDevice_(nullptr)\n> >> \t{\n> >> +\t\thw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hardwareModule_);\n> >\n> > hw_get_module(), if my understanding is correct, ends up dlopen()ing the\n> > module. Is there anything we need to do in the destructor to close the\n> > module ?\n> \n> Your understanding is correct, hw_get_module() ends up doing dlopen().\n> AFAIK, there is no equivalent of release the hardwareModule_. There is\n> no hw_put_module().\n> \n> I've skimmed through some other implementations of HALs (such as\n> gatekeeper) and they indeed call dclose().\n\nLovely API design... *sigh*\n\n> Thanks a lot for bringing this up. I will send a v2.\n> \n> >> \t\tASSERT(hardwareModule_);\n> >> \t}\n> >> \n> >> @@ -85,7 +86,7 @@ public:\n> >> \n> >> private:\n> >> \tconst CameraDevice *const cameraDevice_;\n> >> -\tstruct hw_module_t *const hardwareModule_;\n> >> +\tconst struct hw_module_t *hardwareModule_;\n> >> \tstruct alloc_device_t *allocDevice_;\n> >> };\n> >> \n> >> \n> >> ---\n> >> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca\n> >> change-id: 20230227-android-gralloc-04e31d5edc68","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 5C0A3BE080\n\tfor ;\n\tTue, 28 Feb 2023 23:20:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B30ED626AC;\n\tWed, 1 Mar 2023 00:20:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1DE286267E\n\tfor ;\n\tWed, 1 Mar 2023 00:20:12 +0100 (CET)","from pendragon.ideasonboard.com\n\t(153.162-64-87.adsl-dyn.isp.belgacom.be [87.64.162.153])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 94E8D890;\n\tWed, 1 Mar 2023 00:20:11 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677626413;\n\tbh=4r5ooGkwCN3uRn2wDjxJThgrvHTUg98gSlaXFCM17zs=;\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=Pa4x4QGWvdYuMmSuZs4ro4YA2CbL5W0Q6UmNAYGpEuMe8RooYMWGMZUYGUXQwfcAK\n\t7pF5ksFZlHxVFUovUj7yh3xqRqunEq8rMzh0R2SgNXsW+KghkttOGsZho5HWQp8WeU\n\t2pfMD6OZQH2G+lf72bfOQgbdOdmC5Wjs6Q/5cywTSjaIw0/KlvVrMX21VVeVo2la/h\n\tZoZvi/usii2T29jzDSkONvMbiOBllEx0+7vc40KYdWfWSROJk7yKg3k/sV/+KUzGhX\n\tq9h2mPIG5hlOXwHkfw6i0RmJvfQR1UUQTJlKdXT2WXEw1DLsF/rfl97mPMQwaZfZRC\n\tABiOHRYS8Occg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677626411;\n\tbh=4r5ooGkwCN3uRn2wDjxJThgrvHTUg98gSlaXFCM17zs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TCuj7MKyQU8N8858K62BuyFO+qCu1zGBP5PwHmYl/DhhujMEOLX1Z1XJa8vq5jAkH\n\tguVlMIxwpo7Pk1D38fU2GUzNSLbR8oaHEfOK82MusCQYi5zYtSK9emOUFVWTcRpY9B\n\tiKHmnmOeqVUC8wuRNiQErvACZq5ZxmobSlNTYXYc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TCuj7MKy\"; dkim-atps=neutral","Date":"Wed, 1 Mar 2023 01:20:13 +0200","To":"Mattijs Korpershoek ","Message-ID":"","References":"<20230227-android-gralloc-v1-1-3360fcfa7c0d@baylibre.com>\n\t\n\t<871qm9lx20.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<871qm9lx20.fsf@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","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":26508,"web_url":"https://patchwork.libcamera.org/comment/26508/","msgid":"<87wn403nkm.fsf@baylibre.com>","date":"2023-03-01T08:03:53","subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","submitter":{"id":153,"url":"https://patchwork.libcamera.org/api/people/153/","name":"Mattijs Korpershoek","email":"mkorpershoek@baylibre.com"},"content":"On mer., mars 01, 2023 at 01:20, Laurent Pinchart wrote:\n\n> Hi Mattijs,\n>\n> On Tue, Feb 28, 2023 at 02:49:27PM +0100, Mattijs Korpershoek wrote:\n>> Hi Laurent,\n>> \n>> Thank you for your review.\n>> \n>> On lun., févr. 27, 2023 at 18:52, Laurent Pinchart wrote:\n>> \n>> > Hi Mattijs,\n>> >\n>> > On Mon, Feb 27, 2023 at 09:54:02AM +0100, Mattijs Korpershoek via libcamera-devel wrote:\n>> >> PlatformFrameBufferAllocator is an abstraction over gralloc.\n>> >> Right now hardwareModule_ points towards a CAMERA_HARDWARE_MODULE_ID.\n>> >> \n>> >> When gralloc_open() is called we observe:\n>> >> libcamera: DEBUG HAL camera3_hal.cpp:75 Open camera gpu0\n>> >> libcamera: ERROR Camera camera.cpp:524 Camera in Configured state trying acquire() requiring state Available\n>> >> 01-23 14:14:04.742 370 416 E libcamera: FATAL HAL generic_frame_buffer_allocator.cpp:105 gralloc_open() failed: -87\n>> >> \n>> >> Which is wrong, gralloc_open() is attempting to re-open the camera HAL,\n>> >> instead of the gralloc HAL.\n>> >> \n>> >> Point to a GRALLOC_HARDWARE_MODULE_ID instead so that we can request\n>> >> buffers from gralloc in android.\n>> >\n>> > Good catch !\n>> >\n>> >> Note: this add a new dependency on android's libhardware [1]\n>> >> \n>> >> [1] https://android.googlesource.com/platform/hardware/libhardware\n>> >> \n>> >> Fixes: c58662c5770e (\"android: Introduce PlatformFrameBufferAllocator\")\n>> >> Signed-off-by: Mattijs Korpershoek \n>> >> ---\n>> >> RFC as this adds an additional link-time dependency on an AOSP project\n>> >> (libhardware) and I'm unsure how to handle this.\n>> >> \n>> >> I thought of using the subprojects structure as done for libyuv but\n>> >> since libhardware has other AOSP deps (like libvnksupport) I'm not sure\n>> >> this is appropriate.\n>> >> \n>> >> Any thoughts on this?\n>> >\n>> > We can require linking against AOSP libraries when compiling this code,\n>> > that's not an issue. A subproject isn't needed (and wouldn't be\n>> > practical anyway).\n>> \n>> Thank you, that is helpful.\n>> \n>> Does that mean that the change can be kept as-is, and it's acceptable to\n>> have link errors when building with\n>> -Dandroid=enabled -Dandroid_platform=generic ?\n>\n> It should fail at meson setup time, when creating the libhardware\n> dependency.\n\nIndeed. I forgot about adding the dependency in a meson file for v2.\nsorry for the noise.\n\n>\n>> > How did you test-link this patch ?\n>> \n>> I have link-tested this patch in an Android 12 AOSP-based environment.\n>> \n>> I also build-tested using the meson build system, noticing the link error.\n>> \n>> I work in a BayLibre specific integration, where have generated\n>> Android.bp files with a python script based on the meson files. It\n>> parses the meson.build files to generate Android.bp files.\n>> \n>> We use this to build libcamera, the Android Camera HAL and the cam app.\n>> \n>> As I'm not sure sure the libcamera wants to maintain this, I did not\n>> submit it.\n>\n> For now, our plan is to support building against the NDK, but a script\n> that generates Android.bp files is something that could be interesting.\n\nI think supporting building against the NDK is the way to go as well.\nThe instructions from mesa (https://docs.mesa3d.org/android.html) look\npromising.\n\n> We wouldn't commit the generated files to the repository, but we could\n> host the script until a better option is found. I wonder if it could be\n> implemented as a meson backend...\n\nAgreed that the generated .bp files won't be commited, but having the\nscripts might help the android integrators like myself.\n\nIf I get some time during the coming weeks I will clean them up and\nsubmit them. I don't know meson very well but I will look into that at\nthe same time.\n\n>\n>> If this seems useful for libcamera, I can polish this up and submit it.\n>> \n>> >> ---\n>> >> src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++--\n>> >> 1 file changed, 3 insertions(+), 2 deletions(-)\n>> >> \n>> >> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n>> >> index 3750e1bf52a9..58e6c68c4998 100644\n>> >> --- a/src/android/mm/generic_frame_buffer_allocator.cpp\n>> >> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n>> >> @@ -72,9 +72,10 @@ class PlatformFrameBufferAllocator::Private : public Extensible::Private\n>> >> public:\n>> >> \tPrivate(CameraDevice *const cameraDevice)\n>> >> \t\t: cameraDevice_(cameraDevice),\n>> >> -\t\t hardwareModule_(cameraDevice->camera3Device()->common.module),\n>> >> +\t\t hardwareModule_(nullptr),\n>> >> \t\t allocDevice_(nullptr)\n>> >> \t{\n>> >> +\t\thw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hardwareModule_);\n>> >\n>> > hw_get_module(), if my understanding is correct, ends up dlopen()ing the\n>> > module. Is there anything we need to do in the destructor to close the\n>> > module ?\n>> \n>> Your understanding is correct, hw_get_module() ends up doing dlopen().\n>> AFAIK, there is no equivalent of release the hardwareModule_. There is\n>> no hw_put_module().\n>> \n>> I've skimmed through some other implementations of HALs (such as\n>> gatekeeper) and they indeed call dclose().\n>\n> Lovely API design... *sigh*\n\nYeah, and It does not seem to be documented in hardware.h :(\nTo be faire, hardware.h is very, very old and we should probably move\nthis to using hidl interfaces instead, but that's another topic. I still\nthink that this bugfix is valid as-is.\n\n>\n>> Thanks a lot for bringing this up. I will send a v2.\n>> \n>> >> \t\tASSERT(hardwareModule_);\n>> >> \t}\n>> >> \n>> >> @@ -85,7 +86,7 @@ public:\n>> >> \n>> >> private:\n>> >> \tconst CameraDevice *const cameraDevice_;\n>> >> -\tstruct hw_module_t *const hardwareModule_;\n>> >> +\tconst struct hw_module_t *hardwareModule_;\n>> >> \tstruct alloc_device_t *allocDevice_;\n>> >> };\n>> >> \n>> >> \n>> >> ---\n>> >> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca\n>> >> change-id: 20230227-android-gralloc-04e31d5edc68\n>\n> -- \n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1882EBF415\n\tfor ;\n\tWed, 1 Mar 2023 08:03:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71439626A5;\n\tWed, 1 Mar 2023 09:03:57 +0100 (CET)","from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com\n\t[IPv6:2a00:1450:4864:20::42b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 065BF61EE0\n\tfor ;\n\tWed, 1 Mar 2023 09:03:56 +0100 (CET)","by mail-wr1-x42b.google.com with SMTP id f11so374331wrv.8\n\tfor ;\n\tWed, 01 Mar 2023 00:03:55 -0800 (PST)","from localhost ([2a01:cb19:85e6:1900:2bf7:7388:731d:c4e1])\n\tby smtp.gmail.com with ESMTPSA id\n\ts2-20020a5d6a82000000b002c53cc7504csm12243325wru.78.2023.03.01.00.03.54\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 01 Mar 2023 00:03:54 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677657837;\n\tbh=h/1EhNPuyuzYUvo5Hup4B6+bpq10TPtLONldn/BcCrQ=;\n\th=To:In-Reply-To:References:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=MfOMeBh0VOOXEPUu1+8u45NbUH09fhpwV5rigpZmeWUw/1/CSOWLgYXtNoOgUV/zX\n\tOEj8q0/vjTATD8CrBjt1OwFBHezrC+LK6PB95dZGRWxrLVKuCHuvS46rOQTv/k1fLY\n\tcIxvuubLQNZvw38AsYTduuYbtXlZnCH3Ts6ZHyRM/lqqZIHSY7OXjq7z6s/UtomHac\n\tm98+VQeRPmcug/O+uInYYrlQ/ZnDF4pvELsSdgi1zH6cDKUl31RM5N6sOnR42P7Tk3\n\t+L1FhrnhEsV25AyT8yTg0ZUS4EoZIBhVG33etBG7yGCGyrtLBYDrrLBgakvQ/uALSH\n\twZUUmQCfmnn9w==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112; t=1677657835;\n\th=content-transfer-encoding:mime-version:message-id:date:references\n\t:in-reply-to:subject:cc:to:from:from:to:cc:subject:date:message-id\n\t:reply-to; bh=1DVOYqeBPF3uiMFij+b94KTMrTOJ4AYhNWAIsRZtUJQ=;\n\tb=bDGoXvzwZmxcAV44LVxgsAtFYg530Sv9cu+gaOpsR+rQenegNr11ilowTQVudr3QiM\n\tHydcaZ7u6m1doNed7BaI0FsaAszDlIXbGuAaeBVURrQ4wm8KlvPr2aloRbdUHjrid6yc\n\t6Hvcaik6vSWzgpNZ89/kc18WEi10XI4YV6hN83QQpAif4xKAiEG8H8KQxzCHzThmUNU6\n\tRugUpzTPYM+NDinZalPFdwk113Uo2Iu0rrYa2vqigOldMooT+yfr4i1Mhrx+wFit8VEF\n\tF9PqpxpzFejYYWWi8IIUEP96gTa2CRjTcUpybVV1KwH1afUqr0209OUO3I/8Qoyt5KnJ\n\toOsQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"bDGoXvzw\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1677657835;\n\th=content-transfer-encoding:mime-version:message-id:date:references\n\t:in-reply-to:subject:cc:to:from:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=1DVOYqeBPF3uiMFij+b94KTMrTOJ4AYhNWAIsRZtUJQ=;\n\tb=dr6N3OXvXelxG57C3tgDfC+iPCNcZeLFufuxCyHyKjB5HDTa6t4pS2wHzMZksoSf3q\n\tPLS/ljlYTLIQF2a65FRrRVfuIbtADeE5mFV+3b6cfZSL9uMviKenVRNC83sSp6BChoKx\n\tyJySl0XpAfwC1m19xJVNKmE39m/+5Ffyeqy3RpiWCLBGl59VvgU1lD2rudafR5QM7KHM\n\ttHPrfJQPkcwgdDhAdNGqTrFdFyOiHZU7Z6T9RuqRgsrz2klE+tRSpbNVtzIL1KC+NkEK\n\t/JpGvJ/Wyel7CtMVSYZjsxPuKsMP6fFVA3cG32vSbRPDvWtzGoGfXXqp2RcG2D24SNTt\n\t04FA==","X-Gm-Message-State":"AO0yUKX3pQ9dICQoTs+j+LN+4ACWdN4u4z499HqAqkT+eBTQuMt3A15J\n\tL2lu01WPX/9cliZCuMepgQY2znwkAF5Vc5GzBrA=","X-Google-Smtp-Source":"AK7set8OQx01w3ntCxGRI7qVreFfxBT4OFEUpWYCygfF5clvLE7nvcKohUOqpFpt7eUuBMmmVf3TQA==","X-Received":"by 2002:a5d:4586:0:b0:2c7:1d71:e672 with SMTP id\n\tp6-20020a5d4586000000b002c71d71e672mr3928775wrq.7.1677657835241; \n\tWed, 01 Mar 2023 00:03:55 -0800 (PST)","To":"Laurent Pinchart ","In-Reply-To":"","References":"<20230227-android-gralloc-v1-1-3360fcfa7c0d@baylibre.com>\n\t\n\t<871qm9lx20.fsf@baylibre.com>\n\t","Date":"Wed, 01 Mar 2023 09:03:53 +0100","Message-ID":"<87wn403nkm.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH RFC] android: mm: generic: use\n\tGRALLOC_HARDWARE_MODULE_ID","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":"Mattijs Korpershoek via libcamera-devel\n\t","Reply-To":"Mattijs Korpershoek ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]