[{"id":25831,"web_url":"https://patchwork.libcamera.org/comment/25831/","msgid":"","date":"2022-11-20T20:36:28","subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicholas,\n\nThank you for the patch.\n\nOn Sat, Nov 19, 2022 at 11:06:29PM -0600, Nicholas Roth via libcamera-devel wrote:\n> Currently, they way camera_device.cpp handles file descriptors in a\n\ns/they/the/\n\n> buffer_handle_t is incorrect. This can result in the HAL attempting to\n> treat uninitialized memory like file descriptors, usually resulting in\n> a crash.\n> \n> This patch brings the behavior of camera_device.cpp in line with how\n> CameraBuffer handles file descriptors and planes.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=172\n\nI've replied to the bug report in\nhttps://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to\ninvestigate this further, as numFds != buf.numPlanes() doesn't sound\nright.\n\n> Signed-off-by: Nicholas Roth \n> ---\n> src/android/camera_device.cpp | 16 ++++++++++------\n> 1 file changed, 10 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b20e389b..1459d582 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -781,14 +781,18 @@ CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,\n> \t\treturn nullptr;\n> \t}\n> \n> +\tif (camera3buffer->numFds > 1) {\n> +\t\tLOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> +\t}\n\nThis will break operation on Chrome OS.\n\n> +\n> +\tSharedFD fd{ camera3buffer->data[0] };\n> +\tif (!fd.isValid()) {\n> +\t\tLOG(HAL, Fatal) << \"No valid fd\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> \tstd::vector planes(buf.numPlanes());\n> \tfor (size_t i = 0; i < buf.numPlanes(); ++i) {\n> -\t\tSharedFD fd{ camera3buffer->data[i] };\n> -\t\tif (!fd.isValid()) {\n> -\t\t\tLOG(HAL, Fatal) << \"No valid fd\";\n> -\t\t\treturn nullptr;\n> -\t\t}\n> -\n> \t\tplanes[i].fd = fd;\n> \t\tplanes[i].offset = buf.offset(i);\n> \t\tplanes[i].length = buf.size(i);","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 A4E53BE08B\n\tfor ;\n\tSun, 20 Nov 2022 20:36:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0551632F9;\n\tSun, 20 Nov 2022 21:36:44 +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 49FF763095\n\tfor ;\n\tSun, 20 Nov 2022 21:36:44 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A97E547C;\n\tSun, 20 Nov 2022 21:36:43 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668976605;\n\tbh=b5Qw6ws60CR/e5n5eEJIfbeFtWlmmF3VWtm60KGB05U=;\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=D9ViVJ5Z3KjZ5LfPohXrlABTfl8Yj75OnPZx5CHSS2ozHJPoJ/pyakBA0f33NvUgp\n\tm5ALe6uFwMdtZvwBVr7IM5WCAjVTdKgUf/tSsCILIS8qo0ttc+myUo/VPsJaoXk4Qf\n\tgTA/o5XbfKfwmJyOsidj0doJeMNbWKyGVRod9URUrV7soodw9AjctMenls01fS5iGF\n\tQuP1yLk/cfgzBk828Np1t158PBBSUSzbyTDuP7MNd3/REC9lRxecleAw7akLpSkO/Q\n\tkDRnOYO4AMO1REv786wLwE3VN+bhl/2aSYu2kmvgon+HTVt9k1FvYsKUMDLPYeQVOm\n\tVai3Dn1YE6gcQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1668976603;\n\tbh=b5Qw6ws60CR/e5n5eEJIfbeFtWlmmF3VWtm60KGB05U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TiTVzHoRr9qRBCtLN7Z39bSbawEnCpJvU7Wyr9tfM5v0VsgV6/ucdelPJEyWVK05i\n\t/Y1f+RcwvI5JaFu5zJbezNxkCUg847L0cIUzt9WIFzspCg6wt1W+vr63H08T953rg+\n\tfJ2lghOw//2+Xs58G8uc7rBrBlBqa6XzIsUYGXak="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TiTVzHoR\"; dkim-atps=neutral","Date":"Sun, 20 Nov 2022 22:36:28 +0200","To":"Nicholas Roth ","Message-ID":"","References":"<20221120050629.144188-1-nicholas@rothemail.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221120050629.144188-1-nicholas@rothemail.net>","Subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","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":26107,"web_url":"https://patchwork.libcamera.org/comment/26107/","msgid":"","date":"2022-12-18T20:00:02","subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","submitter":{"id":137,"url":"https://patchwork.libcamera.org/api/people/137/","name":"Nicholas Roth","email":"nicholas@rothemail.net"},"content":"> I've replied to the bug report in\n> https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to\n> investigate this further, as numFds != buf.numPlanes() doesn't sound\n> right.\n\nMy understanding from the differences between\nandroid/mm/generic_camera_buffer.cpp (Android) and\nandroid/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on\nAndroid each handle buffers differently. See also the comment in\ngeneric_camera_buffer.cpp:\n```\n/*\n* As Android doesn't offer an API to query buffer layouts, assume for\n* now that the buffer is backed by a single dmabuf, with planes being\n* stored contiguously.\n*/\n```\nI can't easily find \"cros-camera/camera_buffer_manager.h\" on the\npublic internet so I can't comment about that here.\n\n> This will break operation on Chrome OS.\nIn that case, would you be open to the following?\npseudocode:\n```\nif (camera3buffer->numFds == buf.numPlanes()) {\n // Act as if there's one buffer per plane (old behavior / ChromeOS)\n} else if (camera3buffer->numFds == 1) {\n // Act as if planes are stored contiguously in a single dmabuf (Android)\n} else {\n // Log Fatal: Unsupported state: number of FDs\n{camera3buffer->numFds} is not 1 (contiguous planes) and does not\nmatch number of planes {buf.numPlanes()}. Failed to infer buffer\nlayout.\n}\n```\n\nLet me know!\n\nThanks,\n-Nicholas\n\nP.S. I'm moving, and the initial plan is short-term rentals. I may not\nhave access to my PC for awhile. I've tried to put the steps required\nto replicate my development environment in\nhttps://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit#\nand will continue to be active here. I'm also trying to use an IP-KVM\nto maintain access to my desktop but can't guarantee that will work.","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 2EE18C3200\n\tfor ;\n\tSun, 18 Dec 2022 20:00:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8941E633A3;\n\tSun, 18 Dec 2022 21:00:17 +0100 (CET)","from mail-il1-x135.google.com (mail-il1-x135.google.com\n\t[IPv6:2607:f8b0:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9EEED63399\n\tfor ;\n\tSun, 18 Dec 2022 21:00:15 +0100 (CET)","by mail-il1-x135.google.com with SMTP id d14so3833232ilq.11\n\tfor ;\n\tSun, 18 Dec 2022 12:00:15 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671393617;\n\tbh=8KDEpGoWgShJvE6AySqjNIfpFfxgBTULd7TjFk3mYdw=;\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=LMdV0/Jg5vXWmNq3YUwiw8bE/M6XLWBg7/akNPIxXsGH0PuHOMVKE4+FEkRAWT2lH\n\tV+VDyCTAemHLKpEDz/1/e405xO+uUlJVBg0z97txxsHKXItjZh0qMmWizIyqXqUJvW\n\tZZUXJGoTl2qxJVkYDeac5ruhfQ2dVcUnaq/+m17pUbQkNxgpCwI2IkgEMm7/R98Pmd\n\t2F2yqEjywtxHBuSdRg2+ANLU7xcIVyJ9zBon5td7o7vZIpGEI+35uobimBe4BmpZf8\n\t1iGS9g+Ojmbh5D90Ic/zRD7j1yj1QK4fSs5tAlXYCl+c5t0KynEizeVL8VPJb3cgrA\n\tHp+6ZnZjtROmQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=rothemail-net.20210112.gappssmtp.com; s=20210112;\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=eKPBQGX57xw2cG1GxXL1b3mfL1+UcHzhtv3ZNEGogGM=;\n\tb=BJiaF3TKTKBVNcOOcFs/xRSgtRJSapIjFo8p3IC+7EHZtlKAF+DfWrpoJ41AZt3Luh\n\t/AFiUo6XrXvja4DGtnJ2scnKfWj5mDOea162ExyLEs8lNuIiRjDT231SxrU3dv8FH3s8\n\tGJk+yvgkV1VOVJIXL40dVWVQMsAib4vZqOy2M3SksWxN+1JCYvnwV8XpWcc1UQwSZjEv\n\tAOqaK69Hzr8U0bVAkdLStqAe0ftXoGG62G/Hl6eocuEO0ycNW7FUsecLOTXi/UubN6Zm\n\t+H6y/oFHBMr4aIR+ztgtUy91oc7CElV+Mv1BEqrpbK+HlngZuZd+mP4bJOKs4y7+bLYV\n\tLRnQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=rothemail-net.20210112.gappssmtp.com\n\theader.i=@rothemail-net.20210112.gappssmtp.com header.b=\"BJiaF3TK\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=eKPBQGX57xw2cG1GxXL1b3mfL1+UcHzhtv3ZNEGogGM=;\n\tb=6KOgcEsHkTN5yZMsU/t7rk+73n5IvwY+UiOpu7p1RJTfdUm5UtGpGBYthslLWdRaK5\n\tkZaLcaxWBP7AGPnIDcUf0l4pITaZCnY3FKWFE8jUmCrAiNaprh4NPHDxdPDqB4MxMIm7\n\tfyyr9/FBUPsgzzfVmDet4T8VNsy8HR4+J+N7CE0+ollxJmqmAzwL4T1K3a0+4oWbvhke\n\tRp36oTpd8AJlV8b8Gt2Ab8OSwiqCduwqSVlYeGlw9js6Rvgtj3H9pE10/Ok7OEqIhReU\n\tk58iU0YiXXIM6lxm0uXpoxHE6TUZBdWHkd6LsYDUgFgkdksl/UcOKWl+Mdiz6a3YwNVS\n\tHJTA==","X-Gm-Message-State":"ANoB5pmahFJaQv4WKk2Ki4WH3wPipqKvoLY1LWiRbWv4tNLVJoBTbcDZ\n\tjPY+ofnIdPI2PvzvwAbuVIwt9AjVyfW/slr6nJgin3OgjkbsrCk73Ug=","X-Google-Smtp-Source":"AA0mqf5zT2H5goblhKBLkdocS21PyGxzUVpk51CFlFfKxolBFOHr/6fb9frdWifTfVF4hWHQYZ/WZO7EbfeiVwMgtgo=","X-Received":"by 2002:a92:b0f:0:b0:303:929:dc8d with SMTP id\n\tb15-20020a920b0f000000b003030929dc8dmr29768888ilf.118.1671393613972; \n\tSun, 18 Dec 2022 12:00:13 -0800 (PST)","MIME-Version":"1.0","References":"<20221120050629.144188-1-nicholas@rothemail.net>\n\t","In-Reply-To":"","Date":"Sun, 18 Dec 2022 14:00:02 -0600","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","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":"Nicholas Roth via libcamera-devel ","Reply-To":"Nicholas Roth ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26108,"web_url":"https://patchwork.libcamera.org/comment/26108/","msgid":"","date":"2022-12-19T01:38:05","subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicholas,\n\n(CC'ing Han-Lin and Harvey for the Chrome OS side, as well as Jacopo)\n\nOn Sun, Dec 18, 2022 at 02:00:02PM -0600, Nicholas Roth wrote:\n> > I've replied to the bug report in\n> > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to\n> > investigate this further, as numFds != buf.numPlanes() doesn't sound\n> > right.\n> \n> My understanding from the differences between\n> android/mm/generic_camera_buffer.cpp (Android) and\n> android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on\n> Android each handle buffers differently. See also the comment in\n> generic_camera_buffer.cpp:\n> ```\n> /*\n> * As Android doesn't offer an API to query buffer layouts, assume for\n> * now that the buffer is backed by a single dmabuf, with planes being\n> * stored contiguously.\n> */\n> ```\n\nI didn't interpret this to mean that android would give us a buffer with\nnumFds == 1 even for multiplanar formats, but that all the file\ndescriptors would point to the same dmabuf instance. However, looking at\nthe gralloc implementations for Qualcomm platforms available at\nhttps://android.googlesource.com/platform/hardware/qcom/display/, for\nexample the MSM8996 gralloc (https://android.googlesource.com/platform/hardware/qcom/display/+/refs/heads/master/msm8996/libgralloc/gralloc_priv.h#247),\nthe situation is more complicated. numFds is set to 2, with the first fd\nstoring the image buffer file descriptor (allocated from ION), and the\nsecond fd storing a metadata buffer file descriptor (not entirely sure\nwhat it's used for).\n\nThe only reasonable conclusion is that Android is a mess.\n\n> I can't easily find \"cros-camera/camera_buffer_manager.h\" on the\n> public internet so I can't comment about that here.\n\nIt's available at\nhttps://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/include/cros-camera/camera_buffer_manager.h.\n\n> > This will break operation on Chrome OS.\n>\n> In that case, would you be open to the following?\n> pseudocode:\n> ```\n> if (camera3buffer->numFds == buf.numPlanes()) {\n> // Act as if there's one buffer per plane (old behavior / ChromeOS)\n> } else if (camera3buffer->numFds == 1) {\n> // Act as if planes are stored contiguously in a single dmabuf (Android)\n> } else {\n> // Log Fatal: Unsupported state: number of FDs\n> {camera3buffer->numFds} is not 1 (contiguous planes) and does not\n> match number of planes {buf.numPlanes()}. Failed to infer buffer\n> layout.\n> }\n> ```\n\nThis won't work on Qualcomm platforms, but that's likely something we\ncan ignore for now. I'm also worried about Android platforms that would\nprovide one dmabuf instance (and thus one fd) per plane, which is also\nsomething that we could ignore for now I suppose.\n\nLooking at CameraDevice::createFrameBuffer(), we create a CameraBuffer\ninstance, which abstracts platform-specific buffer management. I would\nlike to use that to provides the fds we need to create the FrameBuffer,\ninstead of implementing platform-specific logic in createFrameBuffer()\ndirectly. We could either extend the class to provides the fds, or\npossibly better, to create a FrameBuffer instance.\n\nAny opinion ?\n\n> Let me know!\n> \n> Thanks,\n> -Nicholas\n> \n> P.S. I'm moving, and the initial plan is short-term rentals. I may not\n> have access to my PC for awhile. I've tried to put the steps required\n> to replicate my development environment in\n> https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit#\n> and will continue to be active here. I'm also trying to use an IP-KVM\n> to maintain access to my desktop but can't guarantee that will work.\n\nThank you for putting those notes together and making them public. It's\nuseful.","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 5835AC3213\n\tfor ;\n\tMon, 19 Dec 2022 01:38:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFF0F633A2;\n\tMon, 19 Dec 2022 02:38:12 +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 2F93A603C6\n\tfor ;\n\tMon, 19 Dec 2022 02:38:11 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65F99825;\n\tMon, 19 Dec 2022 02:38:10 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671413892;\n\tbh=WMJpXlletpFanv0nyo8qwWHX8iYdVyGpm6SrLotffbI=;\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=cciZlNwI+tW485d1j0xh+GZPU4wD8Tk6UthoBl1wRWcRrKn7exo51lDD0fTPmD1hT\n\tBjn+KQMwry24akoe5paqqJ3bCRQDCfy6xIQEONB01/c6t9wFEF5XsL7+zTd7Bnje2e\n\txyCPhlEmS58J6ZmmQ7H0WK3iIV9I+KkP6sxe/5G8gn5buIVncwVPOdjrj1zupuB/nX\n\t45PBmoew7C3k6yL4udtane+7084C94l31u7b0tOVNex6yTmI0fpDbQmaQFwPZqkBFh\n\tWLWOTc6t/h1mkajMkV0hM9nbIy/EK7QZMd/yMRBdOE4/U5wLGNX9l5bUVCB11f01x1\n\tQvovQoAqTbfZA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671413890;\n\tbh=WMJpXlletpFanv0nyo8qwWHX8iYdVyGpm6SrLotffbI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BwUo+g25f3YxPx+MeGboJa8SCwUBd/2KcEubovicxUO3pSxLGytLJWTwm+aijMusm\n\trxAjM8V6fvhvvDmiSH3zxGEUfMY0zgzQHyq/UwruCUiP+CCjOiQCc14Oeoyzy/dmVb\n\tO6bCRL6Pn79zLoyEOhNPEUmHM3SUp32AloLJiSrM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BwUo+g25\"; dkim-atps=neutral","Date":"Mon, 19 Dec 2022 03:38:05 +0200","To":"Nicholas Roth ","Message-ID":"","References":"<20221120050629.144188-1-nicholas@rothemail.net>\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] android: Fix improper file descriptor\n\tenumeration","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":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26109,"web_url":"https://patchwork.libcamera.org/comment/26109/","msgid":"<20221219091550.bchca7hxeyzvkouo@uno.localdomain>","date":"2022-12-19T09:15:50","subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent, Nicholas\n\nOn Mon, Dec 19, 2022 at 03:38:05AM +0200, Laurent Pinchart via libcamera-devel wrote:\n> Hi Nicholas,\n>\n> (CC'ing Han-Lin and Harvey for the Chrome OS side, as well as Jacopo)\n>\n> On Sun, Dec 18, 2022 at 02:00:02PM -0600, Nicholas Roth wrote:\n> > > I've replied to the bug report in\n> > > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to\n> > > investigate this further, as numFds != buf.numPlanes() doesn't sound\n> > > right.\n> >\n> > My understanding from the differences between\n> > android/mm/generic_camera_buffer.cpp (Android) and\n> > android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on\n> > Android each handle buffers differently. See also the comment in\n> > generic_camera_buffer.cpp:\n> > ```\n> > /*\n> > * As Android doesn't offer an API to query buffer layouts, assume for\n> > * now that the buffer is backed by a single dmabuf, with planes being\n> > * stored contiguously.\n> > */\n> > ```\n>\n> I didn't interpret this to mean that android would give us a buffer with\n> numFds == 1 even for multiplanar formats, but that all the file\n> descriptors would point to the same dmabuf instance. However, looking at\n> the gralloc implementations for Qualcomm platforms available at\n> https://android.googlesource.com/platform/hardware/qcom/display/, for\n> example the MSM8996 gralloc (https://android.googlesource.com/platform/hardware/qcom/display/+/refs/heads/master/msm8996/libgralloc/gralloc_priv.h#247),\n> the situation is more complicated. numFds is set to 2, with the first fd\n> storing the image buffer file descriptor (allocated from ION), and the\n> second fd storing a metadata buffer file descriptor (not entirely sure\n> what it's used for).\n>\n> The only reasonable conclusion is that Android is a mess.\n>\n> > I can't easily find \"cros-camera/camera_buffer_manager.h\" on the\n> > public internet so I can't comment about that here.\n>\n> It's available at\n> https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/include/cros-camera/camera_buffer_manager.h.\n>\n> > > This will break operation on Chrome OS.\n> >\n> > In that case, would you be open to the following?\n> > pseudocode:\n> > ```\n> > if (camera3buffer->numFds == buf.numPlanes()) {\n> > // Act as if there's one buffer per plane (old behavior / ChromeOS)\n> > } else if (camera3buffer->numFds == 1) {\n> > // Act as if planes are stored contiguously in a single dmabuf (Android)\n> > } else {\n> > // Log Fatal: Unsupported state: number of FDs\n> > {camera3buffer->numFds} is not 1 (contiguous planes) and does not\n> > match number of planes {buf.numPlanes()}. Failed to infer buffer\n> > layout.\n> > }\n> > ```\n>\n> This won't work on Qualcomm platforms, but that's likely something we\n> can ignore for now. I'm also worried about Android platforms that would\n> provide one dmabuf instance (and thus one fd) per plane, which is also\n> something that we could ignore for now I suppose.\n>\n> Looking at CameraDevice::createFrameBuffer(), we create a CameraBuffer\n> instance, which abstracts platform-specific buffer management. I would\n> like to use that to provides the fds we need to create the FrameBuffer,\n> instead of implementing platform-specific logic in createFrameBuffer()\n> directly. We could either extend the class to provides the fds, or\n> possibly better, to create a FrameBuffer instance.\n>\n> Any opinion ?\n>\n\nI concur with the observation that CameraBuffer should be abstracting\nplatform-specific implementation details from the rest of the Camera\nHAL implementation, possibily with something like\nCameraBuffer::createFrameBuffer(const buffer_handle_t handle,..)\n\nI'm now looking at the ChromeOS CameraBufferManager implementation\nwhich abstracts away from us the need to set the right usages flags\nwhen, in example, importing buffers:\nhttps://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/camera_buffer_manager_impl.cc#671\n\nStandard Android platforms will not have the luxury of having a\ngeneric buffer manager as ChromeOS does, and each implemenation, if my\nunderstanding is correct, will need to encode in the Camera HAL\nknowledge of the platform specificities (such as buffers layout) and\nspecific requirements of the gralloc implementation (usage flags).\n\nWe right now have a cros buffer manager backend and a generic one\nwhich is, well, generic and can't be expected to work on all platforms\nas it is. I presume the direction to take would be to specialize that\nclass to adapt it the platform the HAL is running on.\n\nLet me use the platform Nicholas' working on as an example:\n\nWe know we could use [minigbm + rockchip kernel] for allocating NV12\nbuffers on the display. Let's assume the PinephonePro Waydroid build\nis instrumented to use such component. Should we expect to have an\nsrc/android/mm/rockchip_minigbm_camera_buffer.c implementation that\nadapts to the requirements of the Rockchip's minigbm implementation ?\n\n\n> > Let me know!\n> >\n> > Thanks,\n> > -Nicholas\n> >\n> > P.S. I'm moving, and the initial plan is short-term rentals. I may not\n> > have access to my PC for awhile. I've tried to put the steps required\n> > to replicate my development environment in\n> > https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit#\n> > and will continue to be active here. I'm also trying to use an IP-KVM\n> > to maintain access to my desktop but can't guarantee that will work.\n>\n> Thank you for putting those notes together and making them public. It's\n> useful.\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 97018C3213\n\tfor ;\n\tMon, 19 Dec 2022 09:21:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFDD16339B;\n\tMon, 19 Dec 2022 10:21:42 +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 29B6563398\n\tfor ;\n\tMon, 19 Dec 2022 10:15:54 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6348B825;\n\tMon, 19 Dec 2022 10:15:53 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671441703;\n\tbh=zVOovT2gKpB359M8EMOWey56pLjuyJ8SFIynLJjSaaQ=;\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=XBgO+STOBNI07NyfPpv4jBPvhGn7yiXTRWoj7JY8UlSaLYEe/BQZqr+5efS+gDZ5t\n\td4VfKq9mWWRZwVNxbYFK36JBw0quSfUdEnieBFvS75HQGjQ0hx/ek8KM5F/WPeKLuX\n\tJoF64j7YHg0VvhINXIo6qLwlq2FEefFzIie55fam2417yNjuZDDdQ/caZzApJb2cKm\n\tDbzdX3Ab9lkMvzfmOMm4XViWTAsxKslTYx6/eb/bsATBpwUzgTRKX2Essd/QmNEvxT\n\tK2C65gauntFn6BUtX0wChL/bpO9lc2QX1DeqqYG+r6aHUHTz76AUCSA7m9cYSVABJV\n\t15NX+nqisqsrA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671441353;\n\tbh=zVOovT2gKpB359M8EMOWey56pLjuyJ8SFIynLJjSaaQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GTg99oTwBSo4wvKmGuzGJA9JV6vTA14pZkyTkThrBDdXaXG53mdoaK1i+Xd8KryuW\n\tKweTd+fbydSfKtxPDPkNcFvOqprn8iF+H5kTnPZpxGvW7o8HQdzkIFNg55mc7rp9uy\n\tqhchJCZIF0iSvkm//WMTtvaWRbkHRJ7JofRgR1F4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GTg99oTw\"; dkim-atps=neutral","Date":"Mon, 19 Dec 2022 10:15:50 +0100","To":"Laurent Pinchart ","Message-ID":"<20221219091550.bchca7hxeyzvkouo@uno.localdomain>","References":"<20221120050629.144188-1-nicholas@rothemail.net>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-Mailman-Approved-At":"Mon, 19 Dec 2022 10:21:41 +0100","Subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tNicholas Roth ,\n\tlibcamera-devel@lists.libcamera.org, \n\tHarvey Yang ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26111,"web_url":"https://patchwork.libcamera.org/comment/26111/","msgid":"","date":"2022-12-19T22:38:59","subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Dec 19, 2022 at 10:15:50AM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 19, 2022 at 03:38:05AM +0200, Laurent Pinchart via libcamera-devel wrote:\n> > Hi Nicholas,\n> >\n> > (CC'ing Han-Lin and Harvey for the Chrome OS side, as well as Jacopo)\n> >\n> > On Sun, Dec 18, 2022 at 02:00:02PM -0600, Nicholas Roth wrote:\n> > > > I've replied to the bug report in\n> > > > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to\n> > > > investigate this further, as numFds != buf.numPlanes() doesn't sound\n> > > > right.\n> > >\n> > > My understanding from the differences between\n> > > android/mm/generic_camera_buffer.cpp (Android) and\n> > > android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on\n> > > Android each handle buffers differently. See also the comment in\n> > > generic_camera_buffer.cpp:\n> > > ```\n> > > /*\n> > > * As Android doesn't offer an API to query buffer layouts, assume for\n> > > * now that the buffer is backed by a single dmabuf, with planes being\n> > > * stored contiguously.\n> > > */\n> > > ```\n> >\n> > I didn't interpret this to mean that android would give us a buffer with\n> > numFds == 1 even for multiplanar formats, but that all the file\n> > descriptors would point to the same dmabuf instance. However, looking at\n> > the gralloc implementations for Qualcomm platforms available at\n> > https://android.googlesource.com/platform/hardware/qcom/display/, for\n> > example the MSM8996 gralloc (https://android.googlesource.com/platform/hardware/qcom/display/+/refs/heads/master/msm8996/libgralloc/gralloc_priv.h#247),\n> > the situation is more complicated. numFds is set to 2, with the first fd\n> > storing the image buffer file descriptor (allocated from ION), and the\n> > second fd storing a metadata buffer file descriptor (not entirely sure\n> > what it's used for).\n> >\n> > The only reasonable conclusion is that Android is a mess.\n> >\n> > > I can't easily find \"cros-camera/camera_buffer_manager.h\" on the\n> > > public internet so I can't comment about that here.\n> >\n> > It's available at\n> > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/include/cros-camera/camera_buffer_manager.h.\n> >\n> > > > This will break operation on Chrome OS.\n> > >\n> > > In that case, would you be open to the following?\n> > > pseudocode:\n> > > ```\n> > > if (camera3buffer->numFds == buf.numPlanes()) {\n> > > // Act as if there's one buffer per plane (old behavior / ChromeOS)\n> > > } else if (camera3buffer->numFds == 1) {\n> > > // Act as if planes are stored contiguously in a single dmabuf (Android)\n> > > } else {\n> > > // Log Fatal: Unsupported state: number of FDs\n> > > {camera3buffer->numFds} is not 1 (contiguous planes) and does not\n> > > match number of planes {buf.numPlanes()}. Failed to infer buffer\n> > > layout.\n> > > }\n> > > ```\n> >\n> > This won't work on Qualcomm platforms, but that's likely something we\n> > can ignore for now. I'm also worried about Android platforms that would\n> > provide one dmabuf instance (and thus one fd) per plane, which is also\n> > something that we could ignore for now I suppose.\n> >\n> > Looking at CameraDevice::createFrameBuffer(), we create a CameraBuffer\n> > instance, which abstracts platform-specific buffer management. I would\n> > like to use that to provides the fds we need to create the FrameBuffer,\n> > instead of implementing platform-specific logic in createFrameBuffer()\n> > directly. We could either extend the class to provides the fds, or\n> > possibly better, to create a FrameBuffer instance.\n> >\n> > Any opinion ?\n> \n> I concur with the observation that CameraBuffer should be abstracting\n> platform-specific implementation details from the rest of the Camera\n> HAL implementation, possibily with something like\n> CameraBuffer::createFrameBuffer(const buffer_handle_t handle,..)\n> \n> I'm now looking at the ChromeOS CameraBufferManager implementation\n> which abstracts away from us the need to set the right usages flags\n> when, in example, importing buffers:\n> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/camera_buffer_manager_impl.cc#671\n> \n> Standard Android platforms will not have the luxury of having a\n> generic buffer manager as ChromeOS does, and each implemenation, if my\n> understanding is correct, will need to encode in the Camera HAL\n> knowledge of the platform specificities (such as buffers layout) and\n> specific requirements of the gralloc implementation (usage flags).\n\nUntil Android sort out their mess, I think that's right. I won't hold my\nbreath, one of the big design principles of the operating system is that\nplatform-specific components are meant to be developed together, and\nshare common knowledge through hardcoding assumptions, without any\nstandard API to communicate them. I doubt they will change that anytime\nsoon.\n\n> We right now have a cros buffer manager backend and a generic one\n> which is, well, generic and can't be expected to work on all platforms\n> as it is. I presume the direction to take would be to specialize that\n> class to adapt it the platform the HAL is running on.\n\nI believe so, yes. We may not have to specialize it for every single\nAndroid platform though, there may be some platform-specific data that\nwe could also obtain from the HAL configuration file.\n\n> Let me use the platform Nicholas' working on as an example:\n> \n> We know we could use [minigbm + rockchip kernel] for allocating NV12\n> buffers on the display. Let's assume the PinephonePro Waydroid build\n> is instrumented to use such component. Should we expect to have an\n> src/android/mm/rockchip_minigbm_camera_buffer.c implementation that\n> adapts to the requirements of the Rockchip's minigbm implementation ?\n\nIt depends what those requirements would be. One thing we'll likely have\nto specialize CameraBuffer for is the usage of the fds in the\nnative_handle_t structure. If multiple platform use the same layout,\npossibly with other platform-specific differences that could be encoded\nin a configuration file (such as, for example, which pixel format\nIMPLEMENTATION_DEFINED is mapped to, assuming it doesn't depend too much\non the usage flags), then a single CameraBuffer implementation could be\nused for all those platforms.\n\n> > > Let me know!\n> > >\n> > > Thanks,\n> > > -Nicholas\n> > >\n> > > P.S. I'm moving, and the initial plan is short-term rentals. I may not\n> > > have access to my PC for awhile. I've tried to put the steps required\n> > > to replicate my development environment in\n> > > https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit#\n> > > and will continue to be active here. I'm also trying to use an IP-KVM\n> > > to maintain access to my desktop but can't guarantee that will work.\n> >\n> > Thank you for putting those notes together and making them public. It's\n> > useful.","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 CC250C3213\n\tfor ;\n\tMon, 19 Dec 2022 22:39:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24ACE6339B;\n\tMon, 19 Dec 2022 23:39:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E19CF61508\n\tfor ;\n\tMon, 19 Dec 2022 23:39:04 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CFFF5825;\n\tMon, 19 Dec 2022 23:39:03 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671489547;\n\tbh=fzm6YEDZ1MjueUbc9ARCcmNBPKiPGQWmogBTkKvbCbI=;\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=woZ88hmtNqEntn6M/fbmNYMo6zZUmHlaEj6F8nXXltp7WWcyA4WCKaCJlciTZRUod\n\tTuLWx9ocoDz3fpo9beX3w5bJ+3/kg/NbVmDEtXt7uuePJqXxQRvV27nnI+hUJODWzK\n\tEfrJqz0TCH9MqVH91LuQY+GYkuSez1BE6rhY2ivPjARR4FHCeQT0bNIEjrFv/uwlLo\n\t7NRKfngqj9tCf/i8AeG63azGDn74rVWHqPp6UA6O01UI0LXZAmTGbMt4z6SA+5+Otk\n\tedxqq0xb8UA2rK6pykMtqFSh6LxQu9vr3XkrK5USwDxvRrh0f/0TVEUh73HgdUVihR\n\t/R1b12QWZq/EQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671489544;\n\tbh=fzm6YEDZ1MjueUbc9ARCcmNBPKiPGQWmogBTkKvbCbI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tDYPXk8+jCcmaLR947QY6GDht9HLrimeXPrkpEsHZ3+3H9ZsMLj0GdEHLZawQFyBp\n\t31HUAMnnwHwWUoBXtsvh/dLynO+wBg5jZIQbSDDmuOwnoDNi8f+jG3RKp/2OUXrPQN\n\tsTjWIZFr3s9yYNbuVqmGJJ0kRPAZiXf/QR3hPdvE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tDYPXk8+\"; dkim-atps=neutral","Date":"Tue, 20 Dec 2022 00:38:59 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20221120050629.144188-1-nicholas@rothemail.net>\n\t\n\t\n\t\n\t<20221219091550.bchca7hxeyzvkouo@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221219091550.bchca7hxeyzvkouo@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] android: Fix improper file descriptor\n\tenumeration","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":"Nicholas Roth ,\n\tlibcamera-devel@lists.libcamera.org, \n\tHarvey Yang ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]