[{"id":11577,"web_url":"https://patchwork.libcamera.org/comment/11577/","msgid":"<20200724171645.GS5921@pendragon.ideasonboard.com>","date":"2020-07-24T17:16:45","subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\n(CC'ing Tomasz to get a review from a Chrome OS point of view)\n\nThank you for the patch.\n\nOn Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote:\n> Use lseek to query the length of planes where possible rather than leaving\n> the plane.length as zero, which prevents mapping buffers for software\n> processing.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n> \n> I like this one ;-) We need to know the plane lengths for software\n> streams now, but the correct way to get this would be to talk to the\n> gralloc allocator. This would mean pulling in various extra\n> cros-specific libraries, where instead we can query the size of the\n> dmabuf by getting the offset when we SEEK_END.\n> \n> \n>  src/android/camera_device.cpp | 18 ++++++++++++------\n>  1 file changed, 12 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 538b8ab5da03..6212ccdd61ec 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> -\t\t/*\n> -\t\t * Setting length to zero here is OK as the length is only used\n> -\t\t * to map the memory of the plane. Libcamera do not need to poke\n> -\t\t * at the memory content queued by the HAL.\n> -\t\t */\n> -\t\tplane.length = 0;\n> +\t\toff_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> +\t\tif (length == -1) {\n> +\t\t\t/*\n> +\t\t\t * A zero length plane is not fatal unless the\n> +\t\t\t * FrameBuffer is used for a software stream, libcamera\n> +\t\t\t * itself will not access the internal frame content.\n> +\t\t\t */\n> +\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> +\t\t\tlength = 0;\n\nDo you expect this to happen ? If not, I'd return an error. If yes, an\nerror message would be printed in a loop, which isn't very nice. My\npreference would be to return an error if possible.\n\nOtherwise, nice hack :-)\n\n> +\t\t}\n> +\n> +\t\tplane.length = length;\n>  \t\tplanes.push_back(std::move(plane));\n>  \t}\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E35FBBD878\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 24 Jul 2020 17:16:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DB366121D;\n\tFri, 24 Jul 2020 19:16:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3BAC6039B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 24 Jul 2020 19:16:52 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2C9AA538;\n\tFri, 24 Jul 2020 19:16:52 +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=\"ZUzir7Jr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595611012;\n\tbh=dfEaZMKShfvJBIB3QiuDh44ImWU4qdltavfOQsvS/Yg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZUzir7JrfcU0ZoSEMzDiV0p96kXnxYJ0f7M15C+tt0LOTx8CokVie/LIg9wZk1Fy8\n\teHilo9P0VU/tivJZUUDL2ovKc943f7Y5xRpftmtV0uaWs+xrr4HzwnzgVv0cmgSr+4\n\tY+w48T9TE72g86hGKyT3o6E2TsaZ6FELNUW8a28E=","Date":"Fri, 24 Jul 2020 20:16:45 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20200724171645.GS5921@pendragon.ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-8-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200720224232.153717-8-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"Tomasz Figa <tfiga@google.com>,\n\tlibcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11629,"web_url":"https://patchwork.libcamera.org/comment/11629/","msgid":"<CAAFQd5B+19uTgY=gzd46PNB7RfOnVTND2khCPCbeRgY5pDX+Dg@mail.gmail.com>","date":"2020-07-27T13:45:17","subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Kieran,\n>\n> (CC'ing Tomasz to get a review from a Chrome OS point of view)\n>\n> Thank you for the patch.\n>\n> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote:\n> > Use lseek to query the length of planes where possible rather than leaving\n> > the plane.length as zero, which prevents mapping buffers for software\n> > processing.\n> >\n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > ---\n> >\n> > I like this one ;-) We need to know the plane lengths for software\n> > streams now, but the correct way to get this would be to talk to the\n> > gralloc allocator. This would mean pulling in various extra\n> > cros-specific libraries, where instead we can query the size of the\n> > dmabuf by getting the offset when we SEEK_END.\n> >\n> >\n> >  src/android/camera_device.cpp | 18 ++++++++++++------\n> >  1 file changed, 12 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 538b8ab5da03..6212ccdd61ec 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n> >                       continue;\n> >               }\n> >\n> > -             /*\n> > -              * Setting length to zero here is OK as the length is only used\n> > -              * to map the memory of the plane. Libcamera do not need to poke\n> > -              * at the memory content queued by the HAL.\n> > -              */\n> > -             plane.length = 0;\n> > +             off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > +             if (length == -1) {\n> > +                     /*\n> > +                      * A zero length plane is not fatal unless the\n> > +                      * FrameBuffer is used for a software stream, libcamera\n> > +                      * itself will not access the internal frame content.\n> > +                      */\n> > +                     LOG(HAL, Error) << \"Failed to query plane length\";\n> > +                     length = 0;\n>\n> Do you expect this to happen ? If not, I'd return an error. If yes, an\n> error message would be printed in a loop, which isn't very nice. My\n> preference would be to return an error if possible.\n>\n> Otherwise, nice hack :-)\n\nA zero-length plane would be strange and could suggest that the\nbacking file is not a DMA-buf, so it could indeed make sense to error\nout.\n\nOtherwise, we tend to do this in a number of places in Chrome OS and\nseems to be the official way to query a DMA-buf for its size. However\nI wonder if we shouldn't restore the original position after the seek?\n\nBest regards,\nTomasz","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3E87EBD878\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 27 Jul 2020 13:45:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA22561253;\n\tMon, 27 Jul 2020 15:45:31 +0200 (CEST)","from mail-ed1-x541.google.com (mail-ed1-x541.google.com\n\t[IPv6:2a00:1450:4864:20::541])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA6DD605B2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 15:45:30 +0200 (CEST)","by mail-ed1-x541.google.com with SMTP id di22so5007283edb.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 06:45:30 -0700 (PDT)","from mail-wr1-f53.google.com (mail-wr1-f53.google.com.\n\t[209.85.221.53]) by smtp.gmail.com with ESMTPSA id\n\tm6sm7030027ejq.85.2020.07.27.06.45.29\n\tfor <libcamera-devel@lists.libcamera.org>\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 27 Jul 2020 06:45:29 -0700 (PDT)","by mail-wr1-f53.google.com with SMTP id z18so11392040wrm.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 06:45:29 -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=\"bteQZMm8\"; 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=l+o6kWrUF0CuY7mHv1eLmTurvtd4/UKf+BAFv7xX5FM=;\n\tb=bteQZMm8QOZlfd+aCFrfSRRj7I3PVnqjtsX9AbbfuGoiWju3ROH+qjrhmhndzh8rNR\n\tbt4odbLPwsxTB/JtT5gfcIsdk2AAXVfXgapDQoRWRzI2PpR1dDB4tlE2u7Sbcg52D83s\n\tmgBJzvXfvRavNKZ/SUIQz63wj2LOOO2+XfVIw=","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=l+o6kWrUF0CuY7mHv1eLmTurvtd4/UKf+BAFv7xX5FM=;\n\tb=HKjukDq5NLVGCONqq3gAmlmnd3t02BA63xMrN/QCfVRZSMhEJ0YivB1yNCe4Ewuosc\n\tB+NMmbo0R+gJtyFIQ8IstV2BcD0Xmt2XPZ+Lp9yiZYbWaIgyiwoj0rHXYALQ8vAjLUj/\n\tSRuuh+BGw+6T7pyMQhl1KQPmLQNjcTZ6vwX3zr0jUXyfX1eUKSk58j6oFRLLD84uDHw5\n\t6QVPbz5VxSHDoUmc9fGlv25mjEfOisXs3WPFP3gi87tYZatMp/E+s2al+/dlMTHljTZk\n\tBh5AkDtfyBOSpAIGtxMPjt1RJerWr36IojSz/XbaBgiCTxXk3l+Upapdioj53WMPOzw+\n\tLrrA==","X-Gm-Message-State":"AOAM533/em0Aydw0PJdr6/z/YgSU5LaUIo85XYAZPyuJST4vvjipFRJq\n\tpEPtD5gO7SbW1e291FN2Dcw6XWRetdp9gA==","X-Google-Smtp-Source":"ABdhPJwkp3h5Cr79IkCr3gK7tIxgI41U0+wpU9denQfkUjPe2tEDOytljZBBQsPS8SWwdxOmZBe5Lw==","X-Received":["by 2002:a05:6402:1841:: with SMTP id\n\tv1mr21498482edy.198.1595857529984; \n\tMon, 27 Jul 2020 06:45:29 -0700 (PDT)","by 2002:a5d:6744:: with SMTP id\n\tl4mr19659864wrw.105.1595857528599; \n\tMon, 27 Jul 2020 06:45:28 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-8-kieran.bingham@ideasonboard.com>\n\t<20200724171645.GS5921@pendragon.ideasonboard.com>","In-Reply-To":"<20200724171645.GS5921@pendragon.ideasonboard.com>","From":"Tomasz Figa <tfiga@chromium.org>","Date":"Mon, 27 Jul 2020 15:45:17 +0200","X-Gmail-Original-Message-ID":"<CAAFQd5B+19uTgY=gzd46PNB7RfOnVTND2khCPCbeRgY5pDX+Dg@mail.gmail.com>","Message-ID":"<CAAFQd5B+19uTgY=gzd46PNB7RfOnVTND2khCPCbeRgY5pDX+Dg@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11630,"web_url":"https://patchwork.libcamera.org/comment/11630/","msgid":"<abc95f57-0a28-479f-f0f4-990449260f7e@ideasonboard.com>","date":"2020-07-27T14:21:29","subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi All,\n\nOn 27/07/2020 14:45, Tomasz Figa wrote:\n> On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart\n> <laurent.pinchart@ideasonboard.com> wrote:\n>>\n>> Hi Kieran,\n>>\n>> (CC'ing Tomasz to get a review from a Chrome OS point of view)\n>>\n>> Thank you for the patch.\n>>\n>> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote:\n>>> Use lseek to query the length of planes where possible rather than leaving\n>>> the plane.length as zero, which prevents mapping buffers for software\n>>> processing.\n>>>\n>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>> ---\n>>>\n>>> I like this one ;-) We need to know the plane lengths for software\n>>> streams now, but the correct way to get this would be to talk to the\n>>> gralloc allocator. This would mean pulling in various extra\n>>> cros-specific libraries, where instead we can query the size of the\n>>> dmabuf by getting the offset when we SEEK_END.\n>>>\n>>>\n>>>  src/android/camera_device.cpp | 18 ++++++++++++------\n>>>  1 file changed, 12 insertions(+), 6 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index 538b8ab5da03..6212ccdd61ec 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n>>>                       continue;\n>>>               }\n>>>\n>>> -             /*\n>>> -              * Setting length to zero here is OK as the length is only used\n>>> -              * to map the memory of the plane. Libcamera do not need to poke\n>>> -              * at the memory content queued by the HAL.\n>>> -              */\n>>> -             plane.length = 0;\n>>> +             off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n>>> +             if (length == -1) {\n>>> +                     /*\n>>> +                      * A zero length plane is not fatal unless the\n>>> +                      * FrameBuffer is used for a software stream, libcamera\n>>> +                      * itself will not access the internal frame content.\n>>> +                      */\n>>> +                     LOG(HAL, Error) << \"Failed to query plane length\";\n>>> +                     length = 0;\n>>\n>> Do you expect this to happen ? If not, I'd return an error. If yes, an\n>> error message would be printed in a loop, which isn't very nice. My\n>> preference would be to return an error if possible.\n>>\n\nI don't think I expect it to happen.\n\n>> Otherwise, nice hack :-)\n> \n> A zero-length plane would be strange and could suggest that the\n> backing file is not a DMA-buf, so it could indeed make sense to error\n> out.\n\nI suspect I coded this thinking that 0 was currently acceptable, so it\nmight still be acceptable to use.\n\nBut I agree with you both that if we can't determine the length of the\nbuffer then we have an error and should highlight it accordingly.\n\n\n> Otherwise, we tend to do this in a number of places in Chrome OS and\n> seems to be the official way to query a DMA-buf for its size. However\n> I wonder if we shouldn't restore the original position after the seek?\n\nDoes CrOS do this? It's an extra couple of calls, (SEEK_CUR, and\nSEEK_SET), but I hope they're not too expensive ...\n\n\n\nI would expect anyone using a DMABuf will mmap the data if needed rather\nthan use read()/write(), but that's probably making too big an\nassumption, so indeed if we left it at SEEK_END we might break something ...\n\n--\nKieran\n\n> Best regards,\n> Tomasz\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D4A6FBD86F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 27 Jul 2020 14:21:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CB936138C;\n\tMon, 27 Jul 2020 16:21:33 +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 88AAB6118A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 16:21:32 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D5B8A556;\n\tMon, 27 Jul 2020 16:21:31 +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=\"N8F57gE8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595859692;\n\tbh=ma/dXFUg44//D0MoP8MEWJX6GrMzLZg+T8Bx5X0r/2Q=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=N8F57gE8TBJZNGd2CGbCuO8ystu/G8n/JH97V+q45rQ10r8UFswy+Czt8XQD2bc9T\n\tL/Ox8P5CZetVr4YDYqb+P7lANi1wHf+penhRTfeRSpn0/eD78iMt6M9Ofcpxed6sgW\n\tPI8C5zZewKkwKPhkQzmQMtTMdi+pF8pFlVljtbwU=","To":"Tomasz Figa <tfiga@chromium.org>,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-8-kieran.bingham@ideasonboard.com>\n\t<20200724171645.GS5921@pendragon.ideasonboard.com>\n\t<CAAFQd5B+19uTgY=gzd46PNB7RfOnVTND2khCPCbeRgY5pDX+Dg@mail.gmail.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<abc95f57-0a28-479f-f0f4-990449260f7e@ideasonboard.com>","Date":"Mon, 27 Jul 2020 15:21:29 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<CAAFQd5B+19uTgY=gzd46PNB7RfOnVTND2khCPCbeRgY5pDX+Dg@mail.gmail.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11631,"web_url":"https://patchwork.libcamera.org/comment/11631/","msgid":"<CAAFQd5Bs9Bwij+Fb-aRuTDZFaVO9MHb57ppn=itjem45xwaRDw@mail.gmail.com>","date":"2020-07-27T14:30:08","subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Mon, Jul 27, 2020 at 4:21 PM Kieran Bingham\n<kieran.bingham@ideasonboard.com> wrote:\n>\n> Hi All,\n>\n> On 27/07/2020 14:45, Tomasz Figa wrote:\n> > On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart\n> > <laurent.pinchart@ideasonboard.com> wrote:\n> >>\n> >> Hi Kieran,\n> >>\n> >> (CC'ing Tomasz to get a review from a Chrome OS point of view)\n> >>\n> >> Thank you for the patch.\n> >>\n> >> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote:\n> >>> Use lseek to query the length of planes where possible rather than leaving\n> >>> the plane.length as zero, which prevents mapping buffers for software\n> >>> processing.\n> >>>\n> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>> ---\n> >>>\n> >>> I like this one ;-) We need to know the plane lengths for software\n> >>> streams now, but the correct way to get this would be to talk to the\n> >>> gralloc allocator. This would mean pulling in various extra\n> >>> cros-specific libraries, where instead we can query the size of the\n> >>> dmabuf by getting the offset when we SEEK_END.\n> >>>\n> >>>\n> >>>  src/android/camera_device.cpp | 18 ++++++++++++------\n> >>>  1 file changed, 12 insertions(+), 6 deletions(-)\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index 538b8ab5da03..6212ccdd61ec 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n> >>>                       continue;\n> >>>               }\n> >>>\n> >>> -             /*\n> >>> -              * Setting length to zero here is OK as the length is only used\n> >>> -              * to map the memory of the plane. Libcamera do not need to poke\n> >>> -              * at the memory content queued by the HAL.\n> >>> -              */\n> >>> -             plane.length = 0;\n> >>> +             off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> >>> +             if (length == -1) {\n> >>> +                     /*\n> >>> +                      * A zero length plane is not fatal unless the\n> >>> +                      * FrameBuffer is used for a software stream, libcamera\n> >>> +                      * itself will not access the internal frame content.\n> >>> +                      */\n> >>> +                     LOG(HAL, Error) << \"Failed to query plane length\";\n> >>> +                     length = 0;\n> >>\n> >> Do you expect this to happen ? If not, I'd return an error. If yes, an\n> >> error message would be printed in a loop, which isn't very nice. My\n> >> preference would be to return an error if possible.\n> >>\n>\n> I don't think I expect it to happen.\n>\n> >> Otherwise, nice hack :-)\n> >\n> > A zero-length plane would be strange and could suggest that the\n> > backing file is not a DMA-buf, so it could indeed make sense to error\n> > out.\n>\n> I suspect I coded this thinking that 0 was currently acceptable, so it\n> might still be acceptable to use.\n>\n> But I agree with you both that if we can't determine the length of the\n> buffer then we have an error and should highlight it accordingly.\n>\n>\n> > Otherwise, we tend to do this in a number of places in Chrome OS and\n> > seems to be the official way to query a DMA-buf for its size. However\n> > I wonder if we shouldn't restore the original position after the seek?\n>\n> Does CrOS do this? It's an extra couple of calls, (SEEK_CUR, and\n> SEEK_SET), but I hope they're not too expensive ...\n>\n>\n>\n> I would expect anyone using a DMABuf will mmap the data if needed rather\n> than use read()/write(), but that's probably making too big an\n> assumption, so indeed if we left it at SEEK_END we might break something ...\n\nThe views on this are separated even in Chrome OS. There are places\nwhere we simply set it back to 0 and there are places where we do a\nproper save, change and restore. I don't have a strong opinion on this\nand I'm pretty much sure we don't have any read/write calls to\nDMA-bufs in Chrome OS, but there could be some implications on other\nplatforms, so I've intentionally made the last sentence a question. :)\n\nBest regards,\nTomasz","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C1C18BD878\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 27 Jul 2020 14:30:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D26A613B1;\n\tMon, 27 Jul 2020 16:30:23 +0200 (CEST)","from mail-ej1-x642.google.com (mail-ej1-x642.google.com\n\t[IPv6:2a00:1450:4864:20::642])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9A636118A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 16:30:21 +0200 (CEST)","by mail-ej1-x642.google.com with SMTP id kq25so4347071ejb.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 07:30:21 -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\tv24sm7457902eds.71.2020.07.27.07.30.20\n\tfor <libcamera-devel@lists.libcamera.org>\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 27 Jul 2020 07:30:20 -0700 (PDT)","by mail-wm1-f43.google.com with SMTP id f18so4870602wmc.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 07:30:20 -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=\"V/vli43k\"; 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=A870zlL9LrFmaQGSOhXYd1guYLIjxxtR4v4ET/KVRlg=;\n\tb=V/vli43kSSpL7lEha2SuBXLwK0CSm1c6gtieUHSUHB51vi4382fEAbjeExN6xgCOEJ\n\tYFpUUyM3AwJc3cEpPfeUX0vqhJIgdCFg6xDItNTWeh3MUrVyMcc8pAkIkWQ9PUFbXM0a\n\txtP4jTewtHjju2wZTNlebRZfvf13e7p4UOnyk=","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=A870zlL9LrFmaQGSOhXYd1guYLIjxxtR4v4ET/KVRlg=;\n\tb=fonJqK47jCP6IgNoPQ/TGHPRgJeDm5jLyrZkgIswi0A7Ey432KKLOLjPJL5S1NgJcD\n\tmVb8y8cX7qn9kdr0rYFNWf0yU640yYrMoU/cOuHr7x+lrI7O7Di+zLLiLVM42I1Kolnn\n\t4MJunriYhwt3cYEb2kZJaiMQXnk3Au420/EaT1WnfszKIRtFcR2HvGovIxvDoODzjMyY\n\t95q7LLpQA1MVwvo0f91fBv/Ov+S6rJ0sAhB7mbtzX6VxN7+mHMJvx7I2I2punHTrOYVi\n\to18fgUja/NrziG1rjeft44l9dyN3h/r5aHFoIQMo2iROQsPSG8bnpbvkwx48aJ6Hqy1V\n\tajzA==","X-Gm-Message-State":"AOAM530M4cOmxmAKcQERi+MzkdAjGAPe68XGroJOmLVQJ/b/6N5vuyeT\n\tkqXleXVeDNQ2+LEUXrJOq1YzqaQjPnpw1g==","X-Google-Smtp-Source":"ABdhPJxGwFCbyARVLh7plKXzayhorV5T9bGdj7hYj8EEC031vxAJ5s/YD6KplwQYfuMbwsapfsY5Sg==","X-Received":["by 2002:a17:906:819:: with SMTP id\n\te25mr5556325ejd.95.1595860220962; \n\tMon, 27 Jul 2020 07:30:20 -0700 (PDT)","by 2002:a1c:e382:: with SMTP id\n\ta124mr20841827wmh.11.1595860219732; \n\tMon, 27 Jul 2020 07:30:19 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-8-kieran.bingham@ideasonboard.com>\n\t<20200724171645.GS5921@pendragon.ideasonboard.com>\n\t<CAAFQd5B+19uTgY=gzd46PNB7RfOnVTND2khCPCbeRgY5pDX+Dg@mail.gmail.com>\n\t<abc95f57-0a28-479f-f0f4-990449260f7e@ideasonboard.com>","In-Reply-To":"<abc95f57-0a28-479f-f0f4-990449260f7e@ideasonboard.com>","From":"Tomasz Figa <tfiga@chromium.org>","Date":"Mon, 27 Jul 2020 16:30:08 +0200","X-Gmail-Original-Message-ID":"<CAAFQd5Bs9Bwij+Fb-aRuTDZFaVO9MHb57ppn=itjem45xwaRDw@mail.gmail.com>","Message-ID":"<CAAFQd5Bs9Bwij+Fb-aRuTDZFaVO9MHb57ppn=itjem45xwaRDw@mail.gmail.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11632,"web_url":"https://patchwork.libcamera.org/comment/11632/","msgid":"<20200727145346.GB20890@pendragon.ideasonboard.com>","date":"2020-07-27T14:53:46","subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Mon, Jul 27, 2020 at 04:30:08PM +0200, Tomasz Figa wrote:\n> On Mon, Jul 27, 2020 at 4:21 PM Kieran Bingham wrote:\n> > On 27/07/2020 14:45, Tomasz Figa wrote:\n> >> On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart wrote:\n> >>>\n> >>> Hi Kieran,\n> >>>\n> >>> (CC'ing Tomasz to get a review from a Chrome OS point of view)\n> >>>\n> >>> Thank you for the patch.\n> >>>\n> >>> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote:\n> >>>> Use lseek to query the length of planes where possible rather than leaving\n> >>>> the plane.length as zero, which prevents mapping buffers for software\n> >>>> processing.\n> >>>>\n> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>>> ---\n> >>>>\n> >>>> I like this one ;-) We need to know the plane lengths for software\n> >>>> streams now, but the correct way to get this would be to talk to the\n> >>>> gralloc allocator. This would mean pulling in various extra\n> >>>> cros-specific libraries, where instead we can query the size of the\n> >>>> dmabuf by getting the offset when we SEEK_END.\n> >>>>\n> >>>>\n> >>>>  src/android/camera_device.cpp | 18 ++++++++++++------\n> >>>>  1 file changed, 12 insertions(+), 6 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index 538b8ab5da03..6212ccdd61ec 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n> >>>>                       continue;\n> >>>>               }\n> >>>>\n> >>>> -             /*\n> >>>> -              * Setting length to zero here is OK as the length is only used\n> >>>> -              * to map the memory of the plane. Libcamera do not need to poke\n> >>>> -              * at the memory content queued by the HAL.\n> >>>> -              */\n> >>>> -             plane.length = 0;\n> >>>> +             off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> >>>> +             if (length == -1) {\n> >>>> +                     /*\n> >>>> +                      * A zero length plane is not fatal unless the\n> >>>> +                      * FrameBuffer is used for a software stream, libcamera\n> >>>> +                      * itself will not access the internal frame content.\n> >>>> +                      */\n> >>>> +                     LOG(HAL, Error) << \"Failed to query plane length\";\n> >>>> +                     length = 0;\n> >>>\n> >>> Do you expect this to happen ? If not, I'd return an error. If yes, an\n> >>> error message would be printed in a loop, which isn't very nice. My\n> >>> preference would be to return an error if possible.\n> >>>\n> >\n> > I don't think I expect it to happen.\n> >\n> >>> Otherwise, nice hack :-)\n> >>\n> >> A zero-length plane would be strange and could suggest that the\n> >> backing file is not a DMA-buf, so it could indeed make sense to error\n> >> out.\n> >\n> > I suspect I coded this thinking that 0 was currently acceptable, so it\n> > might still be acceptable to use.\n> >\n> > But I agree with you both that if we can't determine the length of the\n> > buffer then we have an error and should highlight it accordingly.\n> >\n> >> Otherwise, we tend to do this in a number of places in Chrome OS and\n> >> seems to be the official way to query a DMA-buf for its size. However\n> >> I wonder if we shouldn't restore the original position after the seek?\n> >\n> > Does CrOS do this? It's an extra couple of calls, (SEEK_CUR, and\n> > SEEK_SET), but I hope they're not too expensive ...\n> >\n> > I would expect anyone using a DMABuf will mmap the data if needed rather\n> > than use read()/write(), but that's probably making too big an\n> > assumption, so indeed if we left it at SEEK_END we might break something ...\n> \n> The views on this are separated even in Chrome OS. There are places\n> where we simply set it back to 0 and there are places where we do a\n> proper save, change and restore. I don't have a strong opinion on this\n> and I'm pretty much sure we don't have any read/write calls to\n> DMA-bufs in Chrome OS, but there could be some implications on other\n> platforms, so I've intentionally made the last sentence a question. :)\n\ndmabuf doesn't support read/write, so I think it's safe to leave it\nas-is.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 211EDBD86F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 27 Jul 2020 14:53:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9C356138C;\n\tMon, 27 Jul 2020 16:53:55 +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 4B0206118A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jul 2020 16:53:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AB1FA556;\n\tMon, 27 Jul 2020 16:53:53 +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=\"FOWdjVwS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595861633;\n\tbh=Iw2tyTmn8AY6ytWmJJPYUf3pkp6kkbC/b6isleQp1Yc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FOWdjVwSinzWIkmdkflfTsv076WjK+8J8qo3TqSMphN6+hSv9WDb7FILGOWD5zFAd\n\ti5bMyk7qO4m58CqajD/2dxEI8cRbHQg6R08kf5xGwdwlyRJFRQjqIkDtdGv78nyDKZ\n\tUzxPp34D16QHss+2fzAkfDz7s8+PoWiqT7/SVb2U=","Date":"Mon, 27 Jul 2020 17:53:46 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Tomasz Figa <tfiga@chromium.org>","Message-ID":"<20200727145346.GB20890@pendragon.ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-8-kieran.bingham@ideasonboard.com>\n\t<20200724171645.GS5921@pendragon.ideasonboard.com>\n\t<CAAFQd5B+19uTgY=gzd46PNB7RfOnVTND2khCPCbeRgY5pDX+Dg@mail.gmail.com>\n\t<abc95f57-0a28-479f-f0f4-990449260f7e@ideasonboard.com>\n\t<CAAFQd5Bs9Bwij+Fb-aRuTDZFaVO9MHb57ppn=itjem45xwaRDw@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAAFQd5Bs9Bwij+Fb-aRuTDZFaVO9MHb57ppn=itjem45xwaRDw@mail.gmail.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query\n\tplane length","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]