[{"id":19012,"web_url":"https://patchwork.libcamera.org/comment/19012/","msgid":"","date":"2021-08-23T22:08:55","subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Aug 23, 2021 at 10:12:12PM +0900, Hirokazu Honda wrote:\n> This adds offset to FrameBuffer::Plane. It enables representing frame\n> buffers that store planes in the same dmabuf at different offsets, as\n> for instance required by the V4L2 NV12 pixel format.\n> \n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/framebuffer.h | 1 +\n> src/android/mm/generic_camera_buffer.cpp | 2 --\n> src/libcamera/framebuffer.cpp | 27 ++++++++++++++----------\n> 3 files changed, 17 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 28307890..5de3c744 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -42,6 +42,7 @@ class FrameBuffer final : public Extensible\n> public:\n> \tstruct Plane {\n> \t\tFileDescriptor fd;\n> +\t\tunsigned int offset;\n> \t\tunsigned int length;\n> \t};\n> \n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index b296b3e3..73fbd97a 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -52,8 +52,6 @@ private:\n> \tint fd_;\n> \tint flags_;\n> \tstd::vector planeInfo_;\n> -\t/* \\todo remove planes_ is added to MappedBuffer. */\n> -\tstd::vector> planes_;\n\nThis breaks compilation, as the planes_ member is added to MappedBuffer\nin \"libcamera: mapped_framebuffer: Return plane begin address by\nMappedBuffer::maps()\".\n\n> };\n> \n> CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index b401c50e..e9467aa0 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -131,7 +131,7 @@ FrameBuffer::Private::Private()\n> *\n> * The static information describes the memory planes that make a frame. The\n> * planes are specified when creating the FrameBuffer and are expressed as a set\n> - * of dmabuf file descriptors and length.\n> + * of dmabuf file descriptors, offset and length.\n> *\n> * The dynamic information is grouped in a FrameMetadata instance. It is updated\n> * during the processing of a queued capture request, and is valid from the\n> @@ -151,18 +151,18 @@ FrameBuffer::Private::Private()\n> *\n> * Planar pixel formats use multiple memory regions to store the different\n> * colour components of a frame. The Plane structure describes such a memory\n> - * region by a dmabuf file descriptor and a length. A FrameBuffer then\n> - * contains one or multiple planes, depending on the pixel format of the\n> - * frames it is meant to store.\n> + * region by a dmabuf file descriptor, an offset within the dmabuf and a length.\n> + * A FrameBuffer then contains one or multiple planes, depending on the pixel\n> + * format of the frames it is meant to store.\n> *\n> - * To support DMA access, planes are associated with dmabuf objects represented\n> - * by FileDescriptor handles. The Plane class doesn't handle mapping of the\n> - * memory to the CPU, but applications and IPAs may use the dmabuf file\n> - * descriptors to map the plane memory with mmap() and access its contents.\n\nI think this paragraph should be kept, it's still valid.\n\n> + * The offset identifies the location of the plane data from the start of the\n> + * memory referenced by the dmabuf file descriptor. Multiple planes may be\n> + * stored in the same dmabuf, in which case they will reference the same dmabuf\n> + * and different offsets. No two planes may overlap, as specified by their\n> + * offset and length.\n> *\n> - * \\todo Once we have a Kernel API which can express offsets within a plane\n> - * this structure shall be extended to contain this information. See commit\n> - * 83148ce8be55e for initial documentation of this feature.\n> + * \\todo Specify how an application shall decide whether to use a single or\n> + * multiple dmabufs, based on the camera requirements.\n> */\n> \n> /**\n> @@ -170,6 +170,11 @@ FrameBuffer::Private::Private()\n> * \\brief The dmabuf file descriptor\n> */\n> \n> +/**\n> + * \\var FrameBuffer::Plane::offset\n> + * \\brief The plane offset in bytes\n> +*/\n> +\n> /**\n> * \\var FrameBuffer::Plane::length\n> * \\brief The plane length in bytes","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 8C0C3BD87D\n\tfor ;\n\tMon, 23 Aug 2021 22:09:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D43D4688A3;\n\tTue, 24 Aug 2021 00:09:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FCA368890\n\tfor ;\n\tTue, 24 Aug 2021 00:09:06 +0200 (CEST)","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 A91AE2A5;\n\tTue, 24 Aug 2021 00:09:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eYqhAetH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629756545;\n\tbh=C6LGu756vdQBi3NBpY8e/TjW7Ogzmu+dOiN01MkHGvw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eYqhAetHUBwMV/om6KTWA9PIaov250ibaNkYPEHe890klu443tcbi0R5D/3M2pUNX\n\tklnzIAWJUrrJdQhxfs+i8mtYQAp2qyDFtwPTgo7TEOUpbG8/iMIAGkV33PAC5J/qBn\n\tbVHfVqaSIndLyDevBfYj1rxc5NfUk0XhztM+sOpE=","Date":"Tue, 24 Aug 2021 01:08:55 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210823131221.1034059-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","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":19130,"web_url":"https://patchwork.libcamera.org/comment/19130/","msgid":"","date":"2021-08-27T09:49:41","subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Tue, Aug 24, 2021 at 7:09 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 23, 2021 at 10:12:12PM +0900, Hirokazu Honda wrote:\n> > This adds offset to FrameBuffer::Plane. It enables representing frame\n> > buffers that store planes in the same dmabuf at different offsets, as\n> > for instance required by the V4L2 NV12 pixel format.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > include/libcamera/framebuffer.h | 1 +\n> > src/android/mm/generic_camera_buffer.cpp | 2 --\n> > src/libcamera/framebuffer.cpp | 27 ++++++++++++++----------\n> > 3 files changed, 17 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index 28307890..5de3c744 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -42,6 +42,7 @@ class FrameBuffer final : public Extensible\n> > public:\n> > struct Plane {\n> > FileDescriptor fd;\n> > + unsigned int offset;\n> > unsigned int length;\n> > };\n> >\n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index b296b3e3..73fbd97a 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -52,8 +52,6 @@ private:\n> > int fd_;\n> > int flags_;\n> > std::vector planeInfo_;\n> > - /* \\todo remove planes_ is added to MappedBuffer. */\n> > - std::vector> planes_;\n>\n> This breaks compilation, as the planes_ member is added to MappedBuffer\n> in \"libcamera: mapped_framebuffer: Return plane begin address by\n> MappedBuffer::maps()\".\n>\n> > };\n> >\n> > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index b401c50e..e9467aa0 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -131,7 +131,7 @@ FrameBuffer::Private::Private()\n> > *\n> > * The static information describes the memory planes that make a frame. The\n> > * planes are specified when creating the FrameBuffer and are expressed as a set\n> > - * of dmabuf file descriptors and length.\n> > + * of dmabuf file descriptors, offset and length.\n> > *\n> > * The dynamic information is grouped in a FrameMetadata instance. It is updated\n> > * during the processing of a queued capture request, and is valid from the\n> > @@ -151,18 +151,18 @@ FrameBuffer::Private::Private()\n> > *\n> > * Planar pixel formats use multiple memory regions to store the different\n> > * colour components of a frame. The Plane structure describes such a memory\n> > - * region by a dmabuf file descriptor and a length. A FrameBuffer then\n> > - * contains one or multiple planes, depending on the pixel format of the\n> > - * frames it is meant to store.\n> > + * region by a dmabuf file descriptor, an offset within the dmabuf and a length.\n> > + * A FrameBuffer then contains one or multiple planes, depending on the pixel\n> > + * format of the frames it is meant to store.\n> > *\n> > - * To support DMA access, planes are associated with dmabuf objects represented\n> > - * by FileDescriptor handles. The Plane class doesn't handle mapping of the\n> > - * memory to the CPU, but applications and IPAs may use the dmabuf file\n> > - * descriptors to map the plane memory with mmap() and access its contents.\n>\n> I think this paragraph should be kept, it's still valid.\n\nmmap() doesn't always lead to a meaningful view of the buffer. There\nare buffers which use compressed or tiled layouts which have to be\neither decompressed/detiled first or mapped through a translation unit\nwith a specific driver API (often called GTT in the DRM world).\n\nI think it might be a better choice to provide a mapping API and\nhandle those special cases appropriately.\n\n>\n> > + * The offset identifies the location of the plane data from the start of the\n> > + * memory referenced by the dmabuf file descriptor. Multiple planes may be\n> > + * stored in the same dmabuf, in which case they will reference the same dmabuf\n> > + * and different offsets. No two planes may overlap, as specified by their\n> > + * offset and length.\n> > *\n> > - * \\todo Once we have a Kernel API which can express offsets within a plane\n> > - * this structure shall be extended to contain this information. See commit\n> > - * 83148ce8be55e for initial documentation of this feature.\n> > + * \\todo Specify how an application shall decide whether to use a single or\n> > + * multiple dmabufs, based on the camera requirements.\n> > */\n> >\n> > /**\n> > @@ -170,6 +170,11 @@ FrameBuffer::Private::Private()\n> > * \\brief The dmabuf file descriptor\n> > */\n> >\n> > +/**\n> > + * \\var FrameBuffer::Plane::offset\n> > + * \\brief The plane offset in bytes\n> > +*/\n> > +\n> > /**\n> > * \\var FrameBuffer::Plane::length\n> > * \\brief The plane length in bytes\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 9DF72BD87C\n\tfor ;\n\tFri, 27 Aug 2021 09:49:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7F2068932;\n\tFri, 27 Aug 2021 11:49:56 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3BD5568891\n\tfor ;\n\tFri, 27 Aug 2021 11:49:55 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id g22so9033280edy.12\n\tfor ;\n\tFri, 27 Aug 2021 02:49:55 -0700 (PDT)","from mail-wr1-f52.google.com (mail-wr1-f52.google.com.\n\t[209.85.221.52]) by smtp.gmail.com with ESMTPSA id\n\tp8sm2578253ejo.2.2021.08.27.02.49.53\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 27 Aug 2021 02:49:53 -0700 (PDT)","by mail-wr1-f52.google.com with SMTP id b6so9453998wrh.10\n\tfor ;\n\tFri, 27 Aug 2021 02:49:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Lm923WAM\"; 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=TV0ecF+Iq/xDGqFAbSI49K7miLY26ve0ZBeHeNfz/ek=;\n\tb=Lm923WAMUARV8SZSrN8SjOeg3fslvzUiP7W4Ae9ST1b7tnLhO94VA7yC5y8rnlifwS\n\t34xZAiYooyhyF43HEFGrihCKeqw2IZkMgansbmz7neQovbkpT2e1Xn/tbea5I0vIAzHb\n\tUlWTsVsCYomXparOMaE0KtCxqY0QPKGgGKonM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TV0ecF+Iq/xDGqFAbSI49K7miLY26ve0ZBeHeNfz/ek=;\n\tb=jTSP//Kh6zdRMZT95rp+ZeE3ekADMBySNKz8EarLi2zzAs8UoMca9MWIvK7VMIy5nG\n\taR2jgfH4/SA7Fi01Yxshz2PVO0SOySIC2EqDWRXI/fZr2bxOh9twJczPNV6g+3Hq4XWv\n\tjyqyoog6Rl85FX0uVY42dWKHd27ecoP9865aAJU32gg4whNiLIzQbh0lSAqoC8Fm0xKu\n\tY3O7fhHa24ZKrg+EH4aU1xsk6z30qry9UYuIcnonf71sB4XmjixGvxp23E5qfqoH+8ge\n\tMLx5mNAJXneJlCZ5A3rSK7HibrHl6qUG8EOEmDW4ew6bhp/pStKFCnf75gMC6pFVL9ud\n\tU1cQ==","X-Gm-Message-State":"AOAM5335vEHHQDR0cGhpvamVOVFBCBRst4hC2GnTVJcAJ7MKrUo4L+Vq\n\ttrEMgJhYWGkJ6cwlfpWF+OoPJ0IvlgGziA==","X-Google-Smtp-Source":"ABdhPJzRzo8w9rgJvYyOhZXt8yajq7vs/meaOmGF7efiGJZ3usY0eAZSPKGQRthrIh5mr9GVGYqOYQ==","X-Received":["by 2002:a05:6402:310a:: with SMTP id\n\tdc10mr8745487edb.277.1630057794330; \n\tFri, 27 Aug 2021 02:49:54 -0700 (PDT)","by 2002:a5d:538e:: with SMTP id\n\td14mr9232958wrv.192.1630057792933; \n\tFri, 27 Aug 2021 02:49:52 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Fri, 27 Aug 2021 18:49:41 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","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":19176,"web_url":"https://patchwork.libcamera.org/comment/19176/","msgid":"","date":"2021-08-30T14:59:00","subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Fri, Aug 27, 2021 at 06:49:41PM +0900, Tomasz Figa wrote:\n> On Tue, Aug 24, 2021 at 7:09 AM Laurent Pinchart wrote:\n> > On Mon, Aug 23, 2021 at 10:12:12PM +0900, Hirokazu Honda wrote:\n> > > This adds offset to FrameBuffer::Plane. It enables representing frame\n> > > buffers that store planes in the same dmabuf at different offsets, as\n> > > for instance required by the V4L2 NV12 pixel format.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/framebuffer.h | 1 +\n> > > src/android/mm/generic_camera_buffer.cpp | 2 --\n> > > src/libcamera/framebuffer.cpp | 27 ++++++++++++++----------\n> > > 3 files changed, 17 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index 28307890..5de3c744 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -42,6 +42,7 @@ class FrameBuffer final : public Extensible\n> > > public:\n> > > struct Plane {\n> > > FileDescriptor fd;\n> > > + unsigned int offset;\n> > > unsigned int length;\n> > > };\n> > >\n> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > index b296b3e3..73fbd97a 100644\n> > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > @@ -52,8 +52,6 @@ private:\n> > > int fd_;\n> > > int flags_;\n> > > std::vector planeInfo_;\n> > > - /* \\todo remove planes_ is added to MappedBuffer. */\n> > > - std::vector> planes_;\n> >\n> > This breaks compilation, as the planes_ member is added to MappedBuffer\n> > in \"libcamera: mapped_framebuffer: Return plane begin address by\n> > MappedBuffer::maps()\".\n> >\n> > > };\n> > >\n> > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index b401c50e..e9467aa0 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -131,7 +131,7 @@ FrameBuffer::Private::Private()\n> > > *\n> > > * The static information describes the memory planes that make a frame. The\n> > > * planes are specified when creating the FrameBuffer and are expressed as a set\n> > > - * of dmabuf file descriptors and length.\n> > > + * of dmabuf file descriptors, offset and length.\n> > > *\n> > > * The dynamic information is grouped in a FrameMetadata instance. It is updated\n> > > * during the processing of a queued capture request, and is valid from the\n> > > @@ -151,18 +151,18 @@ FrameBuffer::Private::Private()\n> > > *\n> > > * Planar pixel formats use multiple memory regions to store the different\n> > > * colour components of a frame. The Plane structure describes such a memory\n> > > - * region by a dmabuf file descriptor and a length. A FrameBuffer then\n> > > - * contains one or multiple planes, depending on the pixel format of the\n> > > - * frames it is meant to store.\n> > > + * region by a dmabuf file descriptor, an offset within the dmabuf and a length.\n> > > + * A FrameBuffer then contains one or multiple planes, depending on the pixel\n> > > + * format of the frames it is meant to store.\n> > > *\n> > > - * To support DMA access, planes are associated with dmabuf objects represented\n> > > - * by FileDescriptor handles. The Plane class doesn't handle mapping of the\n> > > - * memory to the CPU, but applications and IPAs may use the dmabuf file\n> > > - * descriptors to map the plane memory with mmap() and access its contents.\n> >\n> > I think this paragraph should be kept, it's still valid.\n> \n> mmap() doesn't always lead to a meaningful view of the buffer. There\n> are buffers which use compressed or tiled layouts which have to be\n> either decompressed/detiled first or mapped through a translation unit\n> with a specific driver API (often called GTT in the DRM world).\n> \n> I think it might be a better choice to provide a mapping API and\n> handle those special cases appropriately.\n\nIsn't that the job of a buffer allocator API ? It's a big can of worms,\nand not something specific to libcamera.\n\n> > > + * The offset identifies the location of the plane data from the start of the\n> > > + * memory referenced by the dmabuf file descriptor. Multiple planes may be\n> > > + * stored in the same dmabuf, in which case they will reference the same dmabuf\n> > > + * and different offsets. No two planes may overlap, as specified by their\n> > > + * offset and length.\n> > > *\n> > > - * \\todo Once we have a Kernel API which can express offsets within a plane\n> > > - * this structure shall be extended to contain this information. See commit\n> > > - * 83148ce8be55e for initial documentation of this feature.\n> > > + * \\todo Specify how an application shall decide whether to use a single or\n> > > + * multiple dmabufs, based on the camera requirements.\n> > > */\n> > >\n> > > /**\n> > > @@ -170,6 +170,11 @@ FrameBuffer::Private::Private()\n> > > * \\brief The dmabuf file descriptor\n> > > */\n> > >\n> > > +/**\n> > > + * \\var FrameBuffer::Plane::offset\n> > > + * \\brief The plane offset in bytes\n> > > +*/\n> > > +\n> > > /**\n> > > * \\var FrameBuffer::Plane::length\n> > > * \\brief The plane length in bytes","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 2CBA4BD87D\n\tfor ;\n\tMon, 30 Aug 2021 14:59:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 921A36916A;\n\tMon, 30 Aug 2021 16:59:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B80C60258\n\tfor ;\n\tMon, 30 Aug 2021 16:59:14 +0200 (CEST)","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 9FB035A7;\n\tMon, 30 Aug 2021 16:59:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vkqaSe2s\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630335553;\n\tbh=t6F6L4dEVgRGq8v43dbKZ0xD0YpUmJHZyEPkIYluhbI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vkqaSe2sBve8IDnwzaHv8yr2l2LW0dGq7y49UI1f0F9W3KnwuQh+/iaekKu3GvTtu\n\tbd72ivw/n/UORF2glOgNqBBAJYzf8R7gw6FdunKw0p99Ua9UeOpORBs0SLbvhFa1+q\n\tsIS1mh6yu1cJHPv/4HJ6/71pgS8L/0jYahe51Wt0=","Date":"Mon, 30 Aug 2021 17:59:00 +0300","From":"Laurent Pinchart ","To":"Tomasz Figa ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-2-hiroh@chromium.org>\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] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","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":19194,"web_url":"https://patchwork.libcamera.org/comment/19194/","msgid":"","date":"2021-08-31T02:10:35","subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Mon, Aug 30, 2021 at 11:59 PM Laurent Pinchart\n wrote:\n>\n> Hi Tomasz,\n>\n> On Fri, Aug 27, 2021 at 06:49:41PM +0900, Tomasz Figa wrote:\n> > On Tue, Aug 24, 2021 at 7:09 AM Laurent Pinchart wrote:\n> > > On Mon, Aug 23, 2021 at 10:12:12PM +0900, Hirokazu Honda wrote:\n> > > > This adds offset to FrameBuffer::Plane. It enables representing frame\n> > > > buffers that store planes in the same dmabuf at different offsets, as\n> > > > for instance required by the V4L2 NV12 pixel format.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Laurent Pinchart \n> > > > ---\n> > > > include/libcamera/framebuffer.h | 1 +\n> > > > src/android/mm/generic_camera_buffer.cpp | 2 --\n> > > > src/libcamera/framebuffer.cpp | 27 ++++++++++++++----------\n> > > > 3 files changed, 17 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > index 28307890..5de3c744 100644\n> > > > --- a/include/libcamera/framebuffer.h\n> > > > +++ b/include/libcamera/framebuffer.h\n> > > > @@ -42,6 +42,7 @@ class FrameBuffer final : public Extensible\n> > > > public:\n> > > > struct Plane {\n> > > > FileDescriptor fd;\n> > > > + unsigned int offset;\n> > > > unsigned int length;\n> > > > };\n> > > >\n> > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > index b296b3e3..73fbd97a 100644\n> > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > @@ -52,8 +52,6 @@ private:\n> > > > int fd_;\n> > > > int flags_;\n> > > > std::vector planeInfo_;\n> > > > - /* \\todo remove planes_ is added to MappedBuffer. */\n> > > > - std::vector> planes_;\n> > >\n> > > This breaks compilation, as the planes_ member is added to MappedBuffer\n> > > in \"libcamera: mapped_framebuffer: Return plane begin address by\n> > > MappedBuffer::maps()\".\n> > >\n> > > > };\n> > > >\n> > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > index b401c50e..e9467aa0 100644\n> > > > --- a/src/libcamera/framebuffer.cpp\n> > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > @@ -131,7 +131,7 @@ FrameBuffer::Private::Private()\n> > > > *\n> > > > * The static information describes the memory planes that make a frame. The\n> > > > * planes are specified when creating the FrameBuffer and are expressed as a set\n> > > > - * of dmabuf file descriptors and length.\n> > > > + * of dmabuf file descriptors, offset and length.\n> > > > *\n> > > > * The dynamic information is grouped in a FrameMetadata instance. It is updated\n> > > > * during the processing of a queued capture request, and is valid from the\n> > > > @@ -151,18 +151,18 @@ FrameBuffer::Private::Private()\n> > > > *\n> > > > * Planar pixel formats use multiple memory regions to store the different\n> > > > * colour components of a frame. The Plane structure describes such a memory\n> > > > - * region by a dmabuf file descriptor and a length. A FrameBuffer then\n> > > > - * contains one or multiple planes, depending on the pixel format of the\n> > > > - * frames it is meant to store.\n> > > > + * region by a dmabuf file descriptor, an offset within the dmabuf and a length.\n> > > > + * A FrameBuffer then contains one or multiple planes, depending on the pixel\n> > > > + * format of the frames it is meant to store.\n> > > > *\n> > > > - * To support DMA access, planes are associated with dmabuf objects represented\n> > > > - * by FileDescriptor handles. The Plane class doesn't handle mapping of the\n> > > > - * memory to the CPU, but applications and IPAs may use the dmabuf file\n> > > > - * descriptors to map the plane memory with mmap() and access its contents.\n> > >\n> > > I think this paragraph should be kept, it's still valid.\n> >\n> > mmap() doesn't always lead to a meaningful view of the buffer. There\n> > are buffers which use compressed or tiled layouts which have to be\n> > either decompressed/detiled first or mapped through a translation unit\n> > with a specific driver API (often called GTT in the DRM world).\n> >\n> > I think it might be a better choice to provide a mapping API and\n> > handle those special cases appropriately.\n>\n> Isn't that the job of a buffer allocator API ? It's a big can of worms,\n> and not something specific to libcamera.\n>\n\nWell, isn't it the buffer allocator ultimately who only knows about\nthe nature of the buffer?\n\n> > > > + * The offset identifies the location of the plane data from the start of the\n> > > > + * memory referenced by the dmabuf file descriptor. Multiple planes may be\n> > > > + * stored in the same dmabuf, in which case they will reference the same dmabuf\n> > > > + * and different offsets. No two planes may overlap, as specified by their\n> > > > + * offset and length.\n> > > > *\n> > > > - * \\todo Once we have a Kernel API which can express offsets within a plane\n> > > > - * this structure shall be extended to contain this information. See commit\n> > > > - * 83148ce8be55e for initial documentation of this feature.\n> > > > + * \\todo Specify how an application shall decide whether to use a single or\n> > > > + * multiple dmabufs, based on the camera requirements.\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -170,6 +170,11 @@ FrameBuffer::Private::Private()\n> > > > * \\brief The dmabuf file descriptor\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\var FrameBuffer::Plane::offset\n> > > > + * \\brief The plane offset in bytes\n> > > > +*/\n> > > > +\n> > > > /**\n> > > > * \\var FrameBuffer::Plane::length\n> > > > * \\brief The plane length in bytes\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 C43D4BDC71\n\tfor ;\n\tTue, 31 Aug 2021 02:10:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2705A6916A;\n\tTue, 31 Aug 2021 04:10:51 +0200 (CEST)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD9A568891\n\tfor ;\n\tTue, 31 Aug 2021 04:10:49 +0200 (CEST)","by mail-ed1-x530.google.com with SMTP id d6so24506410edt.7\n\tfor ;\n\tMon, 30 Aug 2021 19:10:49 -0700 (PDT)","from mail-wr1-f42.google.com (mail-wr1-f42.google.com.\n\t[209.85.221.42]) by smtp.gmail.com with ESMTPSA id\n\tgz22sm7695905ejb.15.2021.08.30.19.10.48\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 30 Aug 2021 19:10:48 -0700 (PDT)","by mail-wr1-f42.google.com with SMTP id q11so25041551wrr.9\n\tfor ;\n\tMon, 30 Aug 2021 19:10:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"U6MkczAx\"; 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=259uVzMfn0vFlXS3bqbuqxSSnqquA19ygV6CLXyT2MI=;\n\tb=U6MkczAxFj8x3iMhYONWM/zqOQeKGgf3T2ot5huV1v3AKeHjl5DC2PlWK2ukjnRFRZ\n\tQfbTXIVYFJm+1k5bVGjiHL5Lhr8dfzJ4/+dGBmR8SAEMWYdzBo6irihpFaMDILMnlXlt\n\tMLmUGBJnUI1JK/k+gX/Dilr+r1gRYKMBFJ2gE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=259uVzMfn0vFlXS3bqbuqxSSnqquA19ygV6CLXyT2MI=;\n\tb=ZkD78I929GgjiMRaB8HUXCDYjJ4fwd4ntOYOMsQx8wyaqVN0ucJvI56bXc+zeV+aj2\n\tVvsujn+0ogXOWqHXv7+nRjoNWQLgiDEohfCerKTynPZV5ms1ob9eLbPWn0eiPnSgFBMT\n\tO2tRmCRjXqQy44FRfgWTYa0qRkIQXfewvVE2Xt+scdsUq5421JJY4O02AmhmcNhAMl9y\n\tZO68CLDLqBFfJcYb2ozrjZxZtwA4xZ/Ke9Y4uleg4maOYMgLdsg9UMpR301fizpUXuLy\n\t11Xx4EhMUP0VTT3eRUViw4qJ5p7SXoq2/UN1UoQUC3yviBZHSEn1G/GhS8IAJOSvl8wj\n\tWEWA==","X-Gm-Message-State":"AOAM532/AwZaIDRnxErUuW1/eeyDsWpVwEW2G2J/HHSM0TuX0TEHSaCK\n\tJD8KzcvxsPbV9LgbNxUzffyDL9+etrTRwQ==","X-Google-Smtp-Source":"ABdhPJzI1ktlViMsRMs03ujrn9E/M3+4ZW4fnWfH3Pb+ULez+4xKuvHIX8MPJfdKp7dlB2iidKohdg==","X-Received":["by 2002:a05:6402:318c:: with SMTP id\n\tdi12mr27254381edb.55.1630375848890; \n\tMon, 30 Aug 2021 19:10:48 -0700 (PDT)","by 2002:a05:6000:350:: with SMTP id\n\te16mr7578863wre.197.1630375847463; \n\tMon, 30 Aug 2021 19:10:47 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-2-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Tue, 31 Aug 2021 11:10:35 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","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":19200,"web_url":"https://patchwork.libcamera.org/comment/19200/","msgid":"","date":"2021-08-31T02:43:49","subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Tue, Aug 31, 2021 at 11:10:35AM +0900, Tomasz Figa wrote:\n> On Mon, Aug 30, 2021 at 11:59 PM Laurent Pinchart wrote:\n> > On Fri, Aug 27, 2021 at 06:49:41PM +0900, Tomasz Figa wrote:\n> > > On Tue, Aug 24, 2021 at 7:09 AM Laurent Pinchart wrote:\n> > > > On Mon, Aug 23, 2021 at 10:12:12PM +0900, Hirokazu Honda wrote:\n> > > > > This adds offset to FrameBuffer::Plane. It enables representing frame\n> > > > > buffers that store planes in the same dmabuf at different offsets, as\n> > > > > for instance required by the V4L2 NV12 pixel format.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > ---\n> > > > > include/libcamera/framebuffer.h | 1 +\n> > > > > src/android/mm/generic_camera_buffer.cpp | 2 --\n> > > > > src/libcamera/framebuffer.cpp | 27 ++++++++++++++----------\n> > > > > 3 files changed, 17 insertions(+), 13 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > index 28307890..5de3c744 100644\n> > > > > --- a/include/libcamera/framebuffer.h\n> > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > @@ -42,6 +42,7 @@ class FrameBuffer final : public Extensible\n> > > > > public:\n> > > > > struct Plane {\n> > > > > FileDescriptor fd;\n> > > > > + unsigned int offset;\n> > > > > unsigned int length;\n> > > > > };\n> > > > >\n> > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > index b296b3e3..73fbd97a 100644\n> > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > @@ -52,8 +52,6 @@ private:\n> > > > > int fd_;\n> > > > > int flags_;\n> > > > > std::vector planeInfo_;\n> > > > > - /* \\todo remove planes_ is added to MappedBuffer. */\n> > > > > - std::vector> planes_;\n> > > >\n> > > > This breaks compilation, as the planes_ member is added to MappedBuffer\n> > > > in \"libcamera: mapped_framebuffer: Return plane begin address by\n> > > > MappedBuffer::maps()\".\n> > > >\n> > > > > };\n> > > > >\n> > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > > index b401c50e..e9467aa0 100644\n> > > > > --- a/src/libcamera/framebuffer.cpp\n> > > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > > @@ -131,7 +131,7 @@ FrameBuffer::Private::Private()\n> > > > > *\n> > > > > * The static information describes the memory planes that make a frame. The\n> > > > > * planes are specified when creating the FrameBuffer and are expressed as a set\n> > > > > - * of dmabuf file descriptors and length.\n> > > > > + * of dmabuf file descriptors, offset and length.\n> > > > > *\n> > > > > * The dynamic information is grouped in a FrameMetadata instance. It is updated\n> > > > > * during the processing of a queued capture request, and is valid from the\n> > > > > @@ -151,18 +151,18 @@ FrameBuffer::Private::Private()\n> > > > > *\n> > > > > * Planar pixel formats use multiple memory regions to store the different\n> > > > > * colour components of a frame. The Plane structure describes such a memory\n> > > > > - * region by a dmabuf file descriptor and a length. A FrameBuffer then\n> > > > > - * contains one or multiple planes, depending on the pixel format of the\n> > > > > - * frames it is meant to store.\n> > > > > + * region by a dmabuf file descriptor, an offset within the dmabuf and a length.\n> > > > > + * A FrameBuffer then contains one or multiple planes, depending on the pixel\n> > > > > + * format of the frames it is meant to store.\n> > > > > *\n> > > > > - * To support DMA access, planes are associated with dmabuf objects represented\n> > > > > - * by FileDescriptor handles. The Plane class doesn't handle mapping of the\n> > > > > - * memory to the CPU, but applications and IPAs may use the dmabuf file\n> > > > > - * descriptors to map the plane memory with mmap() and access its contents.\n> > > >\n> > > > I think this paragraph should be kept, it's still valid.\n> > >\n> > > mmap() doesn't always lead to a meaningful view of the buffer. There\n> > > are buffers which use compressed or tiled layouts which have to be\n> > > either decompressed/detiled first or mapped through a translation unit\n> > > with a specific driver API (often called GTT in the DRM world).\n> > >\n> > > I think it might be a better choice to provide a mapping API and\n> > > handle those special cases appropriately.\n> >\n> > Isn't that the job of a buffer allocator API ? It's a big can of worms,\n> > and not something specific to libcamera.\n> \n> Well, isn't it the buffer allocator ultimately who only knows about\n> the nature of the buffer?\n\nExactly my point :-) libcamera imports buffers. It has a convenience API\nto also allocate and export them if the application has no other way to\nallocate buffers, but the main use case is import. When using the camera\nHAL, libcamera operates in import mode only.\n\n> > > > > + * The offset identifies the location of the plane data from the start of the\n> > > > > + * memory referenced by the dmabuf file descriptor. Multiple planes may be\n> > > > > + * stored in the same dmabuf, in which case they will reference the same dmabuf\n> > > > > + * and different offsets. No two planes may overlap, as specified by their\n> > > > > + * offset and length.\n> > > > > *\n> > > > > - * \\todo Once we have a Kernel API which can express offsets within a plane\n> > > > > - * this structure shall be extended to contain this information. See commit\n> > > > > - * 83148ce8be55e for initial documentation of this feature.\n> > > > > + * \\todo Specify how an application shall decide whether to use a single or\n> > > > > + * multiple dmabufs, based on the camera requirements.\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > @@ -170,6 +170,11 @@ FrameBuffer::Private::Private()\n> > > > > * \\brief The dmabuf file descriptor\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\var FrameBuffer::Plane::offset\n> > > > > + * \\brief The plane offset in bytes\n> > > > > +*/\n> > > > > +\n> > > > > /**\n> > > > > * \\var FrameBuffer::Plane::length\n> > > > > * \\brief The plane length in bytes","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 E9CF9BDC71\n\tfor ;\n\tTue, 31 Aug 2021 02:44:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 574D36916A;\n\tTue, 31 Aug 2021 04:44:05 +0200 (CEST)","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 4383D68891\n\tfor ;\n\tTue, 31 Aug 2021 04:44:04 +0200 (CEST)","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 9BFAE323;\n\tTue, 31 Aug 2021 04:44:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j1Y+69B8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630377843;\n\tbh=P6aeGte8N+Nv4o1KVjzBjgfEsfmOz1bRZq2emORbMVg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j1Y+69B8M1BXz8XLhweUZwumJs9L3hK8kAMg8eQQy0fqikwJOyenKxhTGYeVMQKPG\n\tfqH3IfggVRapo0aSVYDTPiW8gfIs/+5psFQLPKLVtQq8PpK7fHX7NQqr4UkM6Sao2h\n\th2BoUg5KEoyjkiYcUicsaq31IuYQ+gEUrtE/XYXM=","Date":"Tue, 31 Aug 2021 05:43:49 +0300","From":"Laurent Pinchart ","To":"Tomasz Figa ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-2-hiroh@chromium.org>\n\t\n\t\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] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","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":19201,"web_url":"https://patchwork.libcamera.org/comment/19201/","msgid":"","date":"2021-08-31T02:59:50","subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Tue, Aug 31, 2021 at 11:44 AM Laurent Pinchart\n wrote:\n>\n> Hi Tomasz,\n>\n> On Tue, Aug 31, 2021 at 11:10:35AM +0900, Tomasz Figa wrote:\n> > On Mon, Aug 30, 2021 at 11:59 PM Laurent Pinchart wrote:\n> > > On Fri, Aug 27, 2021 at 06:49:41PM +0900, Tomasz Figa wrote:\n> > > > On Tue, Aug 24, 2021 at 7:09 AM Laurent Pinchart wrote:\n> > > > > On Mon, Aug 23, 2021 at 10:12:12PM +0900, Hirokazu Honda wrote:\n> > > > > > This adds offset to FrameBuffer::Plane. It enables representing frame\n> > > > > > buffers that store planes in the same dmabuf at different offsets, as\n> > > > > > for instance required by the V4L2 NV12 pixel format.\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > ---\n> > > > > > include/libcamera/framebuffer.h | 1 +\n> > > > > > src/android/mm/generic_camera_buffer.cpp | 2 --\n> > > > > > src/libcamera/framebuffer.cpp | 27 ++++++++++++++----------\n> > > > > > 3 files changed, 17 insertions(+), 13 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > > index 28307890..5de3c744 100644\n> > > > > > --- a/include/libcamera/framebuffer.h\n> > > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > > @@ -42,6 +42,7 @@ class FrameBuffer final : public Extensible\n> > > > > > public:\n> > > > > > struct Plane {\n> > > > > > FileDescriptor fd;\n> > > > > > + unsigned int offset;\n> > > > > > unsigned int length;\n> > > > > > };\n> > > > > >\n> > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > index b296b3e3..73fbd97a 100644\n> > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > @@ -52,8 +52,6 @@ private:\n> > > > > > int fd_;\n> > > > > > int flags_;\n> > > > > > std::vector planeInfo_;\n> > > > > > - /* \\todo remove planes_ is added to MappedBuffer. */\n> > > > > > - std::vector> planes_;\n> > > > >\n> > > > > This breaks compilation, as the planes_ member is added to MappedBuffer\n> > > > > in \"libcamera: mapped_framebuffer: Return plane begin address by\n> > > > > MappedBuffer::maps()\".\n> > > > >\n> > > > > > };\n> > > > > >\n> > > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > > > index b401c50e..e9467aa0 100644\n> > > > > > --- a/src/libcamera/framebuffer.cpp\n> > > > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > > > @@ -131,7 +131,7 @@ FrameBuffer::Private::Private()\n> > > > > > *\n> > > > > > * The static information describes the memory planes that make a frame. The\n> > > > > > * planes are specified when creating the FrameBuffer and are expressed as a set\n> > > > > > - * of dmabuf file descriptors and length.\n> > > > > > + * of dmabuf file descriptors, offset and length.\n> > > > > > *\n> > > > > > * The dynamic information is grouped in a FrameMetadata instance. It is updated\n> > > > > > * during the processing of a queued capture request, and is valid from the\n> > > > > > @@ -151,18 +151,18 @@ FrameBuffer::Private::Private()\n> > > > > > *\n> > > > > > * Planar pixel formats use multiple memory regions to store the different\n> > > > > > * colour components of a frame. The Plane structure describes such a memory\n> > > > > > - * region by a dmabuf file descriptor and a length. A FrameBuffer then\n> > > > > > - * contains one or multiple planes, depending on the pixel format of the\n> > > > > > - * frames it is meant to store.\n> > > > > > + * region by a dmabuf file descriptor, an offset within the dmabuf and a length.\n> > > > > > + * A FrameBuffer then contains one or multiple planes, depending on the pixel\n> > > > > > + * format of the frames it is meant to store.\n> > > > > > *\n> > > > > > - * To support DMA access, planes are associated with dmabuf objects represented\n> > > > > > - * by FileDescriptor handles. The Plane class doesn't handle mapping of the\n> > > > > > - * memory to the CPU, but applications and IPAs may use the dmabuf file\n> > > > > > - * descriptors to map the plane memory with mmap() and access its contents.\n> > > > >\n> > > > > I think this paragraph should be kept, it's still valid.\n> > > >\n> > > > mmap() doesn't always lead to a meaningful view of the buffer. There\n> > > > are buffers which use compressed or tiled layouts which have to be\n> > > > either decompressed/detiled first or mapped through a translation unit\n> > > > with a specific driver API (often called GTT in the DRM world).\n> > > >\n> > > > I think it might be a better choice to provide a mapping API and\n> > > > handle those special cases appropriately.\n> > >\n> > > Isn't that the job of a buffer allocator API ? It's a big can of worms,\n> > > and not something specific to libcamera.\n> >\n> > Well, isn't it the buffer allocator ultimately who only knows about\n> > the nature of the buffer?\n>\n> Exactly my point :-) libcamera imports buffers. It has a convenience API\n> to also allocate and export them if the application has no other way to\n> allocate buffers, but the main use case is import. When using the camera\n> HAL, libcamera operates in import mode only.\n\nRight, but within libcamera we still have consumers which may want to\nmap those buffers, but they are not aware of the allocator itself. So\nwe need some kind of abstraction to get back to the allocator to\nobtain the mapping.\n\n>\n> > > > > > + * The offset identifies the location of the plane data from the start of the\n> > > > > > + * memory referenced by the dmabuf file descriptor. Multiple planes may be\n> > > > > > + * stored in the same dmabuf, in which case they will reference the same dmabuf\n> > > > > > + * and different offsets. No two planes may overlap, as specified by their\n> > > > > > + * offset and length.\n> > > > > > *\n> > > > > > - * \\todo Once we have a Kernel API which can express offsets within a plane\n> > > > > > - * this structure shall be extended to contain this information. See commit\n> > > > > > - * 83148ce8be55e for initial documentation of this feature.\n> > > > > > + * \\todo Specify how an application shall decide whether to use a single or\n> > > > > > + * multiple dmabufs, based on the camera requirements.\n> > > > > > */\n> > > > > >\n> > > > > > /**\n> > > > > > @@ -170,6 +170,11 @@ FrameBuffer::Private::Private()\n> > > > > > * \\brief The dmabuf file descriptor\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\var FrameBuffer::Plane::offset\n> > > > > > + * \\brief The plane offset in bytes\n> > > > > > +*/\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\var FrameBuffer::Plane::length\n> > > > > > * \\brief The plane length in bytes\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 00903BD87D\n\tfor ;\n\tTue, 31 Aug 2021 03:00:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F18AC6916A;\n\tTue, 31 Aug 2021 05:00:05 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A92B468891\n\tfor ;\n\tTue, 31 Aug 2021 05:00:04 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id ia27so35431132ejc.10\n\tfor ;\n\tMon, 30 Aug 2021 20:00:04 -0700 (PDT)","from mail-wm1-f43.google.com (mail-wm1-f43.google.com.\n\t[209.85.128.43]) by smtp.gmail.com with ESMTPSA id\n\tk20sm7382607ejd.33.2021.08.30.20.00.02\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 30 Aug 2021 20:00:03 -0700 (PDT)","by mail-wm1-f43.google.com with SMTP id\n\tc129-20020a1c35870000b02902e6b6135279so1345590wma.0\n\tfor ;\n\tMon, 30 Aug 2021 20:00:02 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"AH1Rp0Qx\"; 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=fMJdPkTuSu58nikuOV9Hj2+IhQOpjk9giXzOcgIcDKg=;\n\tb=AH1Rp0QxVVjeUgdOM54bDmXLDpDWlx5F3ui1o2bC02axBvtN/DzEcNBVzfP+ua2MZK\n\tJVmkE3KWY1yKXsYiJw68tQ5X3pZjWjaQwfv9qxINyNF/2403zOd7EXnJWyF+m788CmXW\n\tmld9BEe+k8oHkPcmHngITGbE3cAVJZFxtqGNk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=fMJdPkTuSu58nikuOV9Hj2+IhQOpjk9giXzOcgIcDKg=;\n\tb=Hgb+DYvUy5fViG4lTT6w5XqUTgi+94zc62xl1YJh/sOOX5EMBhQANb4xB3HGn1z8md\n\tdlcXXJdl/rxocyextU3vccrzkftpgsD54EYMrTFqmYlaJzZyy6e4OTGunUtUq35X0vpe\n\tQUbNjysDXLXDEVORjbwf8ofVjOzZWftISBdezO1eL3vhFMXIXkKCjBoukvnxEAjSN3RX\n\ts2IGDIFgAi4d0tmWYMc2Tf8shSFPvS0raVTGw7KTW47G/hS1ZFRJHJOCUBiTbb6SIlJB\n\tixAnbSk9WtjVpa4KWGAl7+j3rzaNX/C/gwcpx+77TNcW96BUTzVIjRNVDOyUqql6prqL\n\tvdQQ==","X-Gm-Message-State":"AOAM531owJqRrE1avwloxZOju0CIJzGsNgPyDo2HRTvUSTLsj1FSJ6k+\n\t5sJyuL7M0hoZx7n7gmFORvQE0IzWs5yZBQlD","X-Google-Smtp-Source":"ABdhPJxvYN3xnw4uMI63KWomOdbjlzTx+a1xMElnRlzrAdhXw1lT8lwHiJQDBhGevXzCHhXIag0zYw==","X-Received":["by 2002:a17:907:384:: with SMTP id\n\tss4mr28052400ejb.478.1630378803809; \n\tMon, 30 Aug 2021 20:00:03 -0700 (PDT)","by 2002:a7b:cb53:: with SMTP id\n\tv19mr1717144wmj.127.1630378802394; \n\tMon, 30 Aug 2021 20:00:02 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-2-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Tue, 31 Aug 2021 11:59:50 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer:\n\tAdd offset to FrameBuffer::Plane","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\" "}}]