[{"id":25222,"web_url":"https://patchwork.libcamera.org/comment/25222/","msgid":"<20221003075552.sklerclsy2ut5omh@uno.localdomain>","date":"2022-10-03T07:55:52","subject":"Re: [libcamera-devel] [PATCH 0/4] libcamera: Fix kernel deprecation\n\twarning with output buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Sun, Oct 02, 2022 at 03:36:08AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hello,\n>\n> This patch series fixes a warning printed by videobuf2.\n>\n> The V4L2 API specification indicates that, for an output video device,\n> the driver will interpret a zero bytesused value as the buffer length\n> (for v4l2_buffer) or the plane length (for v4l2_plane). The videobuf2\n> framework implements this behaviour, but also considers this case as\n> deprecated and prints a warning:\n>\n> [ 54.375534] use of bytesused == 0 is deprecated and will be removed in the future,\n> [ 54.388026] use the actual size instead.\n\nThis really seems something that should be improved on the kernel\nside. If I got it right, bytesused == 0 was used by old codec drivers\nto signal end of streaming, and a flag has been added to signal to\nvideobuf2 that bytesused == 0 is \"fine\" and maintain compatibility\nwith such drivers.\n\nI wonder if instead we should have a flag to signal to videobuf that\nit should care about bytesused for OUTPUT buffers, so that such old\ncodec drivers can continue using the == 0 trick, and ignore it for all\nother drivers and use the buffer/plane length.\n\nI've cc-ed Hans to know his opinion here..\n\n>\n> This is triggered by the RkISP1 and IPU3 pipeline handlers, which don't\n> set bytesused for the parameters buffers.\n>\n> There are multiple ways to fix this issue, and as I couldn't decide\n> which one is best, I've included two in this series.\n>\n> Patch 1/4 is the simple fix, it sets bytesused to the buffer or plane\n> length before calling VIDIOC_QBUF, essentially replicating in userspace\n> the kernel behaviour that videobuf2 considers deprecated. It may however\n> not be considered right, one may argue that the user of the\n> V4L2VideoDevice should always set the bytesused values correctly.\n>\n\nSince you've already gone for the full solution in patches 2, 3 and\n4...\n\n> Patches 2/4 to 4/4 are an attempt at setting bytesused correctly. As the\n> FrameBuffer metadata is private and only accessible in const form\n> outside of the class (with the exception of V4L2VideoDevice that is\n> listed as a friend), patch 2/4 first makes mutable access to the\n> metadata possible. In order to avoid exposing mutable access to\n> applications, it moves the metadata (and the other remaining private\n> members of the FrameBuffer class) to the FrameBuffer::Private class, and\n> exposes a mutable accessor there. Patches 3/4 and 4/4 then use that\n> accessor to set the bytesused values in the RkISP1 and IPU3 pipeline\n> handlers.\n\n... I don't see why we should instead go for the simple fix if we\nexpect it to fall short as soon as we have variable lenght devices.\nUnless of course, we need a short term fix and it's possible to fix it\non the kernel side...\n\n>\n> One nice side effect of patch 2/4 is that the V4L2VideoDevice class\n> doesn't need to be listed as a friend of the FrameBuffer class anymore.\n> If 1/4 is considered a good fix, I would thus be tempted to still merge\n> 2/4 just to drop the friend statement.\n>\n> 3/4 and 4/4 currently hardcode the bytesused value to the size of the\n> parameters buffer structure. This works fine with the RkISP1 and IPU3\n> pipeline handlers as the parameters buffers have a fixed size, but other\n> devices in the future may use a variable-size buffer which would require\n> the IPA module to pass the data size to the pipeline handler. I was\n> tempted to do the same for RkISP1 and IPU3 for correctness, but decided\n> it was probably not worth it.\n>\n> Note also that, if a future device requires variable-size buffers, the\n> approach in patch 1/4 won't work anymore, the more complex fix will then\n> be required.\n>\n> Laurent Pinchart (4):\n> libcamera: v4l2_videodevice: Ensure non-zero bytesused for output\n> buffers\n> libcamera: framebuffer: Move remaining private data to Private class\n> pipeline: ipu3: Set bytesused before queuing parameters buffer\n> pipeline: rkisp1: Set bytesused before queuing parameters buffer\n>\n> include/libcamera/framebuffer.h | 19 ++----\n> include/libcamera/internal/framebuffer.h | 10 +++-\n> .../mm/cros_frame_buffer_allocator.cpp | 8 +--\n> .../mm/generic_frame_buffer_allocator.cpp | 9 +--\n> src/libcamera/framebuffer.cpp | 58 ++++++++++++-------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++\n> src/libcamera/v4l2_videodevice.cpp | 30 +++++-----\n> 8 files changed, 85 insertions(+), 57 deletions(-)\n>\n>\n> base-commit: 79f0fc937d95cbf1bd39f04dfd8b83206bda5098\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","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 D0ED2BD16B\n\tfor ;\n\tMon, 3 Oct 2022 07:55:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39F8462CD3;\n\tMon, 3 Oct 2022 09:55:56 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E755261F74\n\tfor ;\n\tMon, 3 Oct 2022 09:55:54 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4418C240002;\n\tMon, 3 Oct 2022 07:55:53 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664783756;\n\tbh=IsfkIBDfaohByEEHaJ4R5ndGcjTiHygC+/hSpDXx2OA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=raknx13GOqX9AjqJii5MyBOucOl+yM8KbvAXUg46jIaLVk8WqozX7DAFVTaZmbiiB\n\trVHhM5ooYZ+Ut2ftwEXyCPutJy4eEx4UXtqZOu0toCsuOc4YJ9vEFZNYEPBfXnn229\n\tEaSCg0gVJXtLicYldTykYCq33Bh34h0Xj9Qr83vkR0hgW5wbfhjoO5N+T6a8EUCYDV\n\t1HxXoCI5HfiD7ROdhvfdsadE0tsuRc4AxkwBXbpQsh0E8gRPBXjQIcFKIqKcsBKhxC\n\tEbjOgi/AV3lqFZZeTP13EXrAFnqjeQdEwBePGwtJzQ96eFBVzBp5FhwekVgbmRIH2q\n\ttv7J51ErsElHA==","Date":"Mon, 3 Oct 2022 09:55:52 +0200","To":"Laurent Pinchart ","Message-ID":"<20221003075552.sklerclsy2ut5omh@uno.localdomain>","References":"<20221002003612.13603-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221002003612.13603-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 0/4] libcamera: Fix kernel deprecation\n\twarning with output buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Hans Verkuil , libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25223,"web_url":"https://patchwork.libcamera.org/comment/25223/","msgid":"","date":"2022-10-03T08:05:35","subject":"Re: [libcamera-devel] [PATCH 0/4] libcamera: Fix kernel deprecation\n\twarning with output buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Oct 03, 2022 at 09:55:52AM +0200, Jacopo Mondi wrote:\n> On Sun, Oct 02, 2022 at 03:36:08AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > Hello,\n> >\n> > This patch series fixes a warning printed by videobuf2.\n> >\n> > The V4L2 API specification indicates that, for an output video device,\n> > the driver will interpret a zero bytesused value as the buffer length\n> > (for v4l2_buffer) or the plane length (for v4l2_plane). The videobuf2\n> > framework implements this behaviour, but also considers this case as\n> > deprecated and prints a warning:\n> >\n> > [ 54.375534] use of bytesused == 0 is deprecated and will be removed in the future,\n> > [ 54.388026] use the actual size instead.\n> \n> This really seems something that should be improved on the kernel\n> side. If I got it right, bytesused == 0 was used by old codec drivers\n> to signal end of streaming, and a flag has been added to signal to\n> videobuf2 that bytesused == 0 is \"fine\" and maintain compatibility\n> with such drivers.\n> \n> I wonder if instead we should have a flag to signal to videobuf that\n> it should care about bytesused for OUTPUT buffers, so that such old\n> codec drivers can continue using the == 0 trick, and ignore it for all\n> other drivers and use the buffer/plane length.\n> \n> I've cc-ed Hans to know his opinion here..\n\nI've also raised the issue on the linux-media mailing list:\nhttps://lore.kernel.org/linux-media/YzjR5ajfLfMXvC4D@pendragon.ideasonboard.com/\n\nNonetheless, even if we drop the warning on the kernel side, I think\nlibcamera should pass the correct value in bytesused, in order to avoid\ntriggering the warning on older kernels if nothing else.\n\n> > This is triggered by the RkISP1 and IPU3 pipeline handlers, which don't\n> > set bytesused for the parameters buffers.\n> >\n> > There are multiple ways to fix this issue, and as I couldn't decide\n> > which one is best, I've included two in this series.\n> >\n> > Patch 1/4 is the simple fix, it sets bytesused to the buffer or plane\n> > length before calling VIDIOC_QBUF, essentially replicating in userspace\n> > the kernel behaviour that videobuf2 considers deprecated. It may however\n> > not be considered right, one may argue that the user of the\n> > V4L2VideoDevice should always set the bytesused values correctly.\n> \n> Since you've already gone for the full solution in patches 2, 3 and\n> 4...\n> \n> > Patches 2/4 to 4/4 are an attempt at setting bytesused correctly. As the\n> > FrameBuffer metadata is private and only accessible in const form\n> > outside of the class (with the exception of V4L2VideoDevice that is\n> > listed as a friend), patch 2/4 first makes mutable access to the\n> > metadata possible. In order to avoid exposing mutable access to\n> > applications, it moves the metadata (and the other remaining private\n> > members of the FrameBuffer class) to the FrameBuffer::Private class, and\n> > exposes a mutable accessor there. Patches 3/4 and 4/4 then use that\n> > accessor to set the bytesused values in the RkISP1 and IPU3 pipeline\n> > handlers.\n> \n> ... I don't see why we should instead go for the simple fix if we\n> expect it to fall short as soon as we have variable lenght devices.\n> Unless of course, we need a short term fix and it's possible to fix it\n> on the kernel side...\n\nWorks for me :-) I'll then merge patches 2/4 to 4/4 once they get\nreviewed.\n\n> > One nice side effect of patch 2/4 is that the V4L2VideoDevice class\n> > doesn't need to be listed as a friend of the FrameBuffer class anymore.\n> > If 1/4 is considered a good fix, I would thus be tempted to still merge\n> > 2/4 just to drop the friend statement.\n> >\n> > 3/4 and 4/4 currently hardcode the bytesused value to the size of the\n> > parameters buffer structure. This works fine with the RkISP1 and IPU3\n> > pipeline handlers as the parameters buffers have a fixed size, but other\n> > devices in the future may use a variable-size buffer which would require\n> > the IPA module to pass the data size to the pipeline handler. I was\n> > tempted to do the same for RkISP1 and IPU3 for correctness, but decided\n> > it was probably not worth it.\n> >\n> > Note also that, if a future device requires variable-size buffers, the\n> > approach in patch 1/4 won't work anymore, the more complex fix will then\n> > be required.\n> >\n> > Laurent Pinchart (4):\n> > libcamera: v4l2_videodevice: Ensure non-zero bytesused for output\n> > buffers\n> > libcamera: framebuffer: Move remaining private data to Private class\n> > pipeline: ipu3: Set bytesused before queuing parameters buffer\n> > pipeline: rkisp1: Set bytesused before queuing parameters buffer\n> >\n> > include/libcamera/framebuffer.h | 19 ++----\n> > include/libcamera/internal/framebuffer.h | 10 +++-\n> > .../mm/cros_frame_buffer_allocator.cpp | 8 +--\n> > .../mm/generic_frame_buffer_allocator.cpp | 9 +--\n> > src/libcamera/framebuffer.cpp | 58 ++++++++++++-------\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++\n> > src/libcamera/v4l2_videodevice.cpp | 30 +++++-----\n> > 8 files changed, 85 insertions(+), 57 deletions(-)\n> >\n> >\n> > base-commit: 79f0fc937d95cbf1bd39f04dfd8b83206bda5098","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 0D23CC0DA4\n\tfor ;\n\tMon, 3 Oct 2022 08:05:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E48362CD3;\n\tMon, 3 Oct 2022 10:05:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEABE61F74\n\tfor ;\n\tMon, 3 Oct 2022 10:05:37 +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 03C85A4C;\n\tMon, 3 Oct 2022 10:05:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664784339;\n\tbh=vEfDKKr/LyaayqmnM9bvLnb0qFBcAKSUqu48Lje7O5w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xp+PzeIGUAekocF2Mlx+oqypG4wUw0sHqZlrxzmKSyPmDVDm6C98nawv2ntYjj5va\n\t0pvlKNVZJYrX2sOQHAqylWGWUw+nJ2r5r0yKK0TMNLOTGpODhR96pisLRG8vVchHqr\n\tTnGyqoIW38qSS96hOuUAHmEf1O7pVE9n5qkOkn0T6c/ZGfLqaiYqX2BOKlCD9ENEsX\n\teDWtEiXOrEUlGDmR3q5zJx7FHzD5Qal3N3DSSmusCX6sM2Bh9w7XMHGKRCIT7//atn\n\tdVMjz1yyayu3ftoNqCBoOUe+NvZTp/mSGKf8OSuRZ3RGBWqvz19xfFZ2WKQDqQzZWt\n\t0/BTOOznYx2Tg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664784337;\n\tbh=vEfDKKr/LyaayqmnM9bvLnb0qFBcAKSUqu48Lje7O5w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FjZvntK8QM0T3aSPR+LjoTgwUsCSKRwdUcnMDGVLZjpz2KFdrzt7yYaD7+hLWknsD\n\tPoHU4Wt4B3XkiWQPH+PLaYsc9i8XfB0jVkzwH30RucWTGGmHHd4FxyOj9GogI5x7hO\n\tCfK89/uFum1YkzHNWwdYLEAksFhnLisdTwjDMIC4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FjZvntK8\"; dkim-atps=neutral","Date":"Mon, 3 Oct 2022 11:05:35 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221002003612.13603-1-laurent.pinchart@ideasonboard.com>\n\t<20221003075552.sklerclsy2ut5omh@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221003075552.sklerclsy2ut5omh@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 0/4] libcamera: Fix kernel deprecation\n\twarning with output buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Hans Verkuil , libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]