[{"id":5101,"web_url":"https://patchwork.libcamera.org/comment/5101/","msgid":"<20200606220749.GJ7339@pendragon.ideasonboard.com>","date":"2020-06-06T22:07:49","subject":"Re: [libcamera-devel] [PATCH v2 00/10] libcamera: ipu3: Allow\n\tzero-copy RAW stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patches.\n\nOn Sat, Jun 06, 2020 at 05:04:26PM +0200, Niklas Söderlund wrote:\n> Hi,\n> \n> This series removes the need to copy the buffer when capturing RAW\n> buffers from the IPU3 pipeline. This is made possible by allocating an\n> internal queue of buffers in the pipeline handler which is used when the\n> application does not provide an buffer for the RAW stream.\n> \n> The first 9 out of 10 patches in this series cleans up the IPU3 pipeline\n> handler a but and breaks out the CIO2Device helper class into separate\n> cio.{cpp,h} files. The CIO2Device is also reworked to make its interface\n> more strict, making it easier to grasp what is going in in ipu3.cpp. I\n> plan to post similar patches breaking out the ImgU on top of this\n> series.\n\nI'm not fully convinced by some of the changes to the CIO2Device class,\nas shown in review comments for the corresponding patches. On the other\nhand, I don't see anything that makes the code worse, so I'm not pushing\nback. I think it will be easier to review the CIO2Device abstraction\nwhen the ImgU will be split to a separate file, also with a stricter\ninterface.\n\n> There is on issue with this series. If the camera is configured to\n> supply the application with more then one stream and one of them is a\n> RAW stream. Then the sequence number of the RAW buffers in all requests\n> are set to 0 before the request completes. This is due to that if two or\n> more streams are used the RAW buffer is always queued to the ImgU input\n> to serve as input to the vf and/or output buffers. When the RAW buffers\n> is dequeued from the ImgU input device the kernel sets the sequence\n> number to zero. I believe this is a kernel issue and should be fixed\n> there.\n\nCould you propose a patch ? :-)\n\nRegardless of whether this is a kernel issue or not, I think we'll have\nto rework sequence number handling. It may not make sense to have\nper-buffer sequence numbers, maybe handling that at the request level\nwould be a better option. Or, given that requests are never lost, maybe\nsequence numbers don't make sense and should be removed altogether. This\nneeds to be researched, we haven't really put much effort yet in the\nhandling of dropped frames.\n\n> As the RPi pipeline handler already started using the copy approach we\n> can not yet rename to role nor remove the copyFrom() helper. I aim to\n> work on that once the approach taken in this series is agreed upon.\n> \n> Niklas Söderlund (10):\n> libcamera: stream: Initialize stride and bufferCount\n> libcamera: ipu3: Remove id from camera names\n> libcamera: ipu3: Fold mediaBusToFormat() into only caller\n> libcamera: ipu3: Breakout stream assignment to new function\n> libcamera: ipu3: Calculate number of buffers for ImgU\n> libcamera: ipu3: cio2: Move the CIO2Device class to separate files\n> libcamera: ipu3: cio2: Add function to generate configuration\n> libcamera: ipu3: cio2: Make the V4L2 devices private\n> libcamera: ipu3: cio2: Hide buffer allocation and freeing from users\n> libcamera: ipu3: Allow zero-copy RAW stream capture\n> \n> src/libcamera/pipeline/ipu3/cio2.cpp | 314 +++++++++++++++\n> src/libcamera/pipeline/ipu3/cio2.h | 68 ++++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 486 +++++-------------------\n> src/libcamera/pipeline/ipu3/meson.build | 1 +\n> src/libcamera/stream.cpp | 5 +-\n> 5 files changed, 478 insertions(+), 396 deletions(-)\n> create mode 100644 src/libcamera/pipeline/ipu3/cio2.cpp\n> create mode 100644 src/libcamera/pipeline/ipu3/cio2.h\n>","headers":{"Return-Path":"","Received":["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 742A4603CA\n\tfor ;\n\tSun, 7 Jun 2020 00:08:08 +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 F3C6030D;\n\tSun, 7 Jun 2020 00:08:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T5/Y/Lo9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591481288;\n\tbh=9OfWZysuv2XKTdiSk1KUNGZ3MEFpbvscNuBM++lXhos=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T5/Y/Lo9t/GkcvsgkHKfd8qkOb/NfKWJXw2wpI556nPWVy9wz1jd4wTa8nW4OFuJ9\n\tAY7uTmtiUtT3+p4MjWbYfTi29DYioct/mDp1hPUL1uuk6DILTNHuU3hN9DPz2X70yZ\n\tJu/bACrmEaG18nEAGVeaPAxLgYksMoPnliYSV0Xc=","Date":"Sun, 7 Jun 2020 01:07:49 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200606220749.GJ7339@pendragon.ideasonboard.com>","References":"<20200606150436.1851700-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200606150436.1851700-1-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 00/10] libcamera: ipu3: Allow\n\tzero-copy RAW stream","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","X-List-Received-Date":"Sat, 06 Jun 2020 22:08:08 -0000"}}]