[{"id":20182,"web_url":"https://patchwork.libcamera.org/comment/20182/","msgid":"<20211013114650.tpuocgafv2ljxmcp@uno.localdomain>","date":"2021-10-13T11:46:50","subject":"Re: [libcamera-devel] [RFC PATCH 0/2] android: Fix the issue with\n\tType::Mapped only request","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Sep 27, 2021 at 07:48:19PM +0900, Hirokazu Honda wrote:\n> We have the problem that no buffer is not provided to a native camera\n> if a request given in CameraDevice::processCaptureRequest() contains\n> only streams that are resolved as CameraStream::Type::Mapped in\n> CameraDevice::configureStreams(). See the example in below.\n>\n> ```\n> [0:27:06.863311142] [15203] DEBUG HAL camera_device.cpp:980\n> '\\_SB_.PCI0.I2C2.CAM0': Queueing request 139090704005584 with 2\n> streams\n> [0:27:06.863389502] [15203] DEBUG HAL camera_device.cpp:1005\n> '\\_SB_.PCI0.I2C2.CAM0': 0 - (320x240)[0x00000022] -> (320x240)[NV12]\n> (mapped)\n> [0:27:06.863436331] [15203] DEBUG HAL camera_device.cpp:1005\n> '\\_SB_.PCI0.I2C2.CAM0': 1 - (320x240)[0x00000021] -> (320x240)[NV12]\n> (mapped)\n> [0:27:06.863487886] [15203] DEBUG HAL camera_device.cpp:1043\n> '\\_SB_.PCI0.I2C2.CAM0': buffers=0\n> [0:27:06.863561912] [15218] ERROR Camera camera.cpp:1031 Request\n> contains no buffers\n> ```\n>\n> This patch series fixes the issue by looking up the stream\n> alternatively sent to a native camera. To do that, we have to\n> allocate a FrameBuffer for the stream. One possible way is to use\n> FrameBufferAllocator. However, FrameBufferAllocator::allocate must\n> be called when the state of Camera is Configured, but not Running.\n> So we have to allocate buffers during configureStreams(). But\n> allocating buffers for CameraStrems whose type is Direct wastes\n> memory as they tends to not be used at all.\n>\n> I would rather have a new class that allocates FrameBuffers in a\n> platform-dependent way, cros::CameraBufferManager on ChromeOS and\n> gralloc API on other platforms.\n\nHere is the reply to my previous question.\n\nAnd I actually think the contrary: this series fixes a bug it\nshouldn't introduce new features. I would make sure the minimum amount\nof changes to get there, instead of piling more stuff on top that\nrequires more review, more test, more verifications and could\npotentially introduce new bugs.\n\n>\n> With this patch, android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces passed.\n\nPlease add the full CTS results to the cover letters of series that\ntouch the HAL in non trivial ways.\n\nI think we'll make this a requirement going forward.\n\n>\n> Hirokazu Honda (2):\n> android: Introduce PlatformFrameBufferAllocator\n> android: Send alternative stream configuration if no buffer to be sent\n> exists\n>\n> src/android/camera_device.cpp | 50 ++++++++--\n> src/android/camera_device.h | 4 +\n> src/android/camera_stream.cpp | 37 +++++++-\n> src/android/camera_stream.h | 10 +-\n> src/android/frame_buffer.h | 53 +++++++++++\n> src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> src/android/mm/meson.build | 6 +-\n> 8 files changed, 344 insertions(+), 17 deletions(-)\n> create mode 100644 src/android/frame_buffer.h\n> create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> create mode 100644 src/android/mm/generic_frame_buffer.cpp\n>\n> --\n> 2.33.0.685.g46640cef36-goog","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 78360C323E\n\tfor ;\n\tWed, 13 Oct 2021 11:46:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1647068F4F;\n\tWed, 13 Oct 2021 13:46:05 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5ADD60501\n\tfor ;\n\tWed, 13 Oct 2021 13:46:03 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 1ABE51BF209;\n\tWed, 13 Oct 2021 11:46:02 +0000 (UTC)"],"Date":"Wed, 13 Oct 2021 13:46:50 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211013114650.tpuocgafv2ljxmcp@uno.localdomain>","References":"<20210927104821.2526508-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927104821.2526508-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 0/2] android: Fix the issue with\n\tType::Mapped only request","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20523,"web_url":"https://patchwork.libcamera.org/comment/20523/","msgid":"<20211026091931.hco4yypaaficwx3f@uno.localdomain>","date":"2021-10-26T09:19:31","subject":"Re: [libcamera-devel] [RFC PATCH 0/2] android: Fix the issue with\n\tType::Mapped only request","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Oct 13, 2021 at 01:46:50PM +0200, Jacopo Mondi wrote:\n> Hi Hiro,\n>\n> On Mon, Sep 27, 2021 at 07:48:19PM +0900, Hirokazu Honda wrote:\n> > We have the problem that no buffer is not provided to a native camera\n> > if a request given in CameraDevice::processCaptureRequest() contains\n> > only streams that are resolved as CameraStream::Type::Mapped in\n> > CameraDevice::configureStreams(). See the example in below.\n> >\n> > ```\n> > [0:27:06.863311142] [15203] DEBUG HAL camera_device.cpp:980\n> > '\\_SB_.PCI0.I2C2.CAM0': Queueing request 139090704005584 with 2\n> > streams\n> > [0:27:06.863389502] [15203] DEBUG HAL camera_device.cpp:1005\n> > '\\_SB_.PCI0.I2C2.CAM0': 0 - (320x240)[0x00000022] -> (320x240)[NV12]\n> > (mapped)\n> > [0:27:06.863436331] [15203] DEBUG HAL camera_device.cpp:1005\n> > '\\_SB_.PCI0.I2C2.CAM0': 1 - (320x240)[0x00000021] -> (320x240)[NV12]\n> > (mapped)\n> > [0:27:06.863487886] [15203] DEBUG HAL camera_device.cpp:1043\n> > '\\_SB_.PCI0.I2C2.CAM0': buffers=0\n> > [0:27:06.863561912] [15218] ERROR Camera camera.cpp:1031 Request\n> > contains no buffers\n> > ```\n> >\n> > This patch series fixes the issue by looking up the stream\n> > alternatively sent to a native camera. To do that, we have to\n> > allocate a FrameBuffer for the stream. One possible way is to use\n> > FrameBufferAllocator. However, FrameBufferAllocator::allocate must\n> > be called when the state of Camera is Configured, but not Running.\n> > So we have to allocate buffers during configureStreams(). But\n> > allocating buffers for CameraStrems whose type is Direct wastes\n> > memory as they tends to not be used at all.\n> >\n> > I would rather have a new class that allocates FrameBuffers in a\n> > platform-dependent way, cros::CameraBufferManager on ChromeOS and\n> > gralloc API on other platforms.\n>\n> Here is the reply to my previous question.\n>\n> And I actually think the contrary: this series fixes a bug it\n> shouldn't introduce new features. I would make sure the minimum amount\n> of changes to get there, instead of piling more stuff on top that\n> requires more review, more test, more verifications and could\n> potentially introduce new bugs.\n>\n\nWhat's your take on this ?\n\nA possible way forward could be to separate the upstreaming of the\nplatform allocator, which seems in quite good shape already. It can be\nused to replace the usage of FrameBufferAllocator in CameraStream and\nverified with the current code base.\n\nThen on top we can enable the correct use of the YUV post-processor\nwith your 2/2.\n\nWhat do you think ?\n\nThanks\n j\n\n> >\n> > With this patch, android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces passed.\n>\n> Please add the full CTS results to the cover letters of series that\n> touch the HAL in non trivial ways.\n>\n> I think we'll make this a requirement going forward.\n>\n> >\n> > Hirokazu Honda (2):\n> > android: Introduce PlatformFrameBufferAllocator\n> > android: Send alternative stream configuration if no buffer to be sent\n> > exists\n> >\n> > src/android/camera_device.cpp | 50 ++++++++--\n> > src/android/camera_device.h | 4 +\n> > src/android/camera_stream.cpp | 37 +++++++-\n> > src/android/camera_stream.h | 10 +-\n> > src/android/frame_buffer.h | 53 +++++++++++\n> > src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> > src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> > src/android/mm/meson.build | 6 +-\n> > 8 files changed, 344 insertions(+), 17 deletions(-)\n> > create mode 100644 src/android/frame_buffer.h\n> > create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> > create mode 100644 src/android/mm/generic_frame_buffer.cpp\n> >\n> > --\n> > 2.33.0.685.g46640cef36-goog","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 23D89BF415\n\tfor ;\n\tTue, 26 Oct 2021 09:18:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7702664878;\n\tTue, 26 Oct 2021 11:18:42 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B55660123\n\tfor ;\n\tTue, 26 Oct 2021 11:18:41 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id E539FFF810;\n\tTue, 26 Oct 2021 09:18:40 +0000 (UTC)"],"Date":"Tue, 26 Oct 2021 11:19:31 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211026091931.hco4yypaaficwx3f@uno.localdomain>","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20211013114650.tpuocgafv2ljxmcp@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211013114650.tpuocgafv2ljxmcp@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 0/2] android: Fix the issue with\n\tType::Mapped only request","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20526,"web_url":"https://patchwork.libcamera.org/comment/20526/","msgid":"","date":"2021-10-26T09:25:22","subject":"Re: [libcamera-devel] [RFC PATCH 0/2] android: Fix the issue with\n\tType::Mapped only request","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nThe way has a problem about the number of allocated buffers, here.\n> + LOG(HAL, Error) << \"getBuffer@@@@@ \" << numBuffers;\n> + // numBuffers is 4 (=config.bufferCount) is not enough sadly...\n> + // Should we dynamically allocate?\n\nI am going to allocate every buffer by FrameBufferAllocator.\nThat is, allocate() returns std::unique_ptr.\nI have to declare a class deriving FrameBufferAllocator so that I can\nlet the additional resource be released when the returned FrameBuffer\nis released.\nBut I cannot because FrameBuffer is marked final.\nI am waiting for Laurent's answer if I can drop it.\nhttps://patchwork.libcamera.org/patch/13944/\n\nThanks,\n-Hiro\n\nOn Tue, Oct 26, 2021 at 6:18 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Oct 13, 2021 at 01:46:50PM +0200, Jacopo Mondi wrote:\n> > Hi Hiro,\n> >\n> > On Mon, Sep 27, 2021 at 07:48:19PM +0900, Hirokazu Honda wrote:\n> > > We have the problem that no buffer is not provided to a native camera\n> > > if a request given in CameraDevice::processCaptureRequest() contains\n> > > only streams that are resolved as CameraStream::Type::Mapped in\n> > > CameraDevice::configureStreams(). See the example in below.\n> > >\n> > > ```\n> > > [0:27:06.863311142] [15203] DEBUG HAL camera_device.cpp:980\n> > > '\\_SB_.PCI0.I2C2.CAM0': Queueing request 139090704005584 with 2\n> > > streams\n> > > [0:27:06.863389502] [15203] DEBUG HAL camera_device.cpp:1005\n> > > '\\_SB_.PCI0.I2C2.CAM0': 0 - (320x240)[0x00000022] -> (320x240)[NV12]\n> > > (mapped)\n> > > [0:27:06.863436331] [15203] DEBUG HAL camera_device.cpp:1005\n> > > '\\_SB_.PCI0.I2C2.CAM0': 1 - (320x240)[0x00000021] -> (320x240)[NV12]\n> > > (mapped)\n> > > [0:27:06.863487886] [15203] DEBUG HAL camera_device.cpp:1043\n> > > '\\_SB_.PCI0.I2C2.CAM0': buffers=0\n> > > [0:27:06.863561912] [15218] ERROR Camera camera.cpp:1031 Request\n> > > contains no buffers\n> > > ```\n> > >\n> > > This patch series fixes the issue by looking up the stream\n> > > alternatively sent to a native camera. To do that, we have to\n> > > allocate a FrameBuffer for the stream. One possible way is to use\n> > > FrameBufferAllocator. However, FrameBufferAllocator::allocate must\n> > > be called when the state of Camera is Configured, but not Running.\n> > > So we have to allocate buffers during configureStreams(). But\n> > > allocating buffers for CameraStrems whose type is Direct wastes\n> > > memory as they tends to not be used at all.\n> > >\n> > > I would rather have a new class that allocates FrameBuffers in a\n> > > platform-dependent way, cros::CameraBufferManager on ChromeOS and\n> > > gralloc API on other platforms.\n> >\n> > Here is the reply to my previous question.\n> >\n> > And I actually think the contrary: this series fixes a bug it\n> > shouldn't introduce new features. I would make sure the minimum amount\n> > of changes to get there, instead of piling more stuff on top that\n> > requires more review, more test, more verifications and could\n> > potentially introduce new bugs.\n> >\n>\n> What's your take on this ?\n>\n> A possible way forward could be to separate the upstreaming of the\n> platform allocator, which seems in quite good shape already. It can be\n> used to replace the usage of FrameBufferAllocator in CameraStream and\n> verified with the current code base.\n>\n> Then on top we can enable the correct use of the YUV post-processor\n> with your 2/2.\n>\n> What do you think ?\n>\n> Thanks\n> j\n>\n> > >\n> > > With this patch, android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces passed.\n> >\n> > Please add the full CTS results to the cover letters of series that\n> > touch the HAL in non trivial ways.\n> >\n> > I think we'll make this a requirement going forward.\n> >\n> > >\n> > > Hirokazu Honda (2):\n> > > android: Introduce PlatformFrameBufferAllocator\n> > > android: Send alternative stream configuration if no buffer to be sent\n> > > exists\n> > >\n> > > src/android/camera_device.cpp | 50 ++++++++--\n> > > src/android/camera_device.h | 4 +\n> > > src/android/camera_stream.cpp | 37 +++++++-\n> > > src/android/camera_stream.h | 10 +-\n> > > src/android/frame_buffer.h | 53 +++++++++++\n> > > src/android/mm/cros_frame_buffer.cpp | 85 +++++++++++++++++\n> > > src/android/mm/generic_frame_buffer.cpp | 116 ++++++++++++++++++++++++\n> > > src/android/mm/meson.build | 6 +-\n> > > 8 files changed, 344 insertions(+), 17 deletions(-)\n> > > create mode 100644 src/android/frame_buffer.h\n> > > create mode 100644 src/android/mm/cros_frame_buffer.cpp\n> > > create mode 100644 src/android/mm/generic_frame_buffer.cpp\n> > >\n> > > --\n> > > 2.33.0.685.g46640cef36-goog","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 1BCBCBF415\n\tfor ;\n\tTue, 26 Oct 2021 09:25:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D44F46487A;\n\tTue, 26 Oct 2021 11:25:34 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DC8160123\n\tfor ;\n\tTue, 26 Oct 2021 11:25:33 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id z20so11439395edc.13\n\tfor ;\n\tTue, 26 Oct 2021 02:25:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"U96XoHMj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=kBdVrE6sFdcZdE+v2k4iegWNed/RYIXKVU9HN0Z7M6E=;\n\tb=U96XoHMj/ucdCMLTcdoCMaB3xXXDYywVzSdQCqhxR2bO8yV4WSlWCiwQSR4EEdaxlW\n\tkmDIM9OnZkpMGAGA9hvYNfKOiFJHlJfwTmnfmlgdY0gLaHk+KAAdgLY4vx0J2HMO0noh\n\tmW9fxI3GkyMmpaa8WE/Cq5x33Eu00L5ygBSX8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=kBdVrE6sFdcZdE+v2k4iegWNed/RYIXKVU9HN0Z7M6E=;\n\tb=jZ0PcozT03I58opQ5mUK6DEsD8THje+2jjIBp+PF6YEilHfKvKgTnnNso98w7ZxEl0\n\tdkySOfpM/+El7Ny6VAGOlYoC2CD5BHIjnDLK6T/oayND9OPm2DPFhBfpsPhrgxmzbqgB\n\tS7lur7bF3r+Pttm43wgHYy1zD+UZ/efnE71LUKru7X29R3hTLRS/ukzqASTvW7Af28xw\n\tOQJ8uD6QHYKIGF6DP1aq8rVULKMg+3j37Hr6GqWmbziK9Zt70CG8smzMmY7IX1LaS/nI\n\tDqpfdAhv5TkXTUNfrdvA4Rh7RSX934bVhhR/VD7pO9A9Clr/r1cakabI68WfBISfZ2A3\n\tlwqw==","X-Gm-Message-State":"AOAM530zneePmQYhWrr1AILzeecH/OrQ71SC6ez6OWWXXYZ+jBYpYVJn\n\tBiS7AHoSyufxWxYNcgjsJMOAQ/RljmvM5heeWL2Qgom36D7B6w==","X-Google-Smtp-Source":"ABdhPJyouchcZPVWu5uG87J0grD0qzt0lUNJrXIvI4qFPFd8a7JPgNgg3xzam7vdMSj+uqpAcIm6WlOgxHh2drofWNc=","X-Received":"by 2002:a17:906:7313:: with SMTP id\n\tdi19mr27935198ejc.522.1635240332845; \n\tTue, 26 Oct 2021 02:25:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20211013114650.tpuocgafv2ljxmcp@uno.localdomain>\n\t<20211026091931.hco4yypaaficwx3f@uno.localdomain>","In-Reply-To":"<20211026091931.hco4yypaaficwx3f@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 26 Oct 2021 18:25:22 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 0/2] android: Fix the issue with\n\tType::Mapped only request","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]