[{"id":30411,"web_url":"https://patchwork.libcamera.org/comment/30411/","msgid":"","date":"2024-07-16T09:18:12","subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Hans\n\nOn Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote:\n> V4L2VideoDevice is using the caps to determine which kind of buffers to\n> use with the video-device in 2 different cases:\n>\n> 1. V4L2VideoDevice::open()\n> 2. V4L2VideoDevice::[get|try|set]Format()\n>\n> And the order in which the caps are checked is different between\n> these 2 cases. This is a problem for /dev/video# nodes which support\n> both video-capture and metadata buffers. open() sets bufferType_ to\n> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as\n> [get|try|set]Format() will call [get|set]FormatMeta() which does not\n> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.\n>\n> Switch [get|try|set]Format() to use the bufferType_ to determine on what\n> sort of buffers they should be operating, leaving the V4L2VideoDevice\n> code with only a single place where the decision is made what sort\n> of buffers it should operate on for a specific /dev/video# node.\n>\n> This will also allow to modify open() in the future to take a bufferType\n> argument to allow overriding the default bufferType it selects for\n> /dev/video# nodes which are capable of supporting more then 1 buffer type.\n\nDoes this mean that, at the moment, on a device that supports both\ncapture and metadata buffer types, video-capture is selected\nunconditionally and the metadata interface cannot be used ?\n\nIf you plan to add an option to override the bufferType_ at open()\ntime, does it mean it is needed to go through a open-close-open cycle\nto switch buffer type or:\n\n1) The device can be opened multiple times and bufferType_ gets\noverriden ? (I'm not sure this is a good ideas as the last caller of\nopen() selects the bufferType_)\n\n2) Should we instead allow overriding the bufferType_ to use with an\noptional parameter to [get|try|set]Format() instead ?\n\n>\n> Signed-off-by: Hans de Goede \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------\n> 1 file changed, 41 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 4947aa3d..a8dc5355 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const\n> */\n> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> {\n> -\tif (caps_.isMeta())\n> -\t\treturn getFormatMeta(format);\n> -\telse if (caps_.isMultiplanar())\n> -\t\treturn getFormatMultiplane(format);\n> -\telse\n> +\tswitch (bufferType_) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> \t\treturn getFormatSingleplane(format);\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> +\t\treturn getFormatMultiplane(format);\n> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> +\t\treturn getFormatMeta(format);\n> +\tdefault:\n> +\t\treturn -EINVAL;\n> +\t}\n> }\n>\n> /**\n> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> */\n> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> {\n> -\tif (caps_.isMeta())\n> -\t\treturn trySetFormatMeta(format, false);\n> -\telse if (caps_.isMultiplanar())\n> -\t\treturn trySetFormatMultiplane(format, false);\n> -\telse\n> +\tswitch (bufferType_) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> \t\treturn trySetFormatSingleplane(format, false);\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> +\t\treturn trySetFormatMultiplane(format, false);\n> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> +\t\treturn trySetFormatMeta(format, false);\n> +\tdefault:\n> +\t\treturn -EINVAL;\n> +\t}\n> }\n>\n> /**\n> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> {\n> \tint ret = 0;\n\nI don't think it is necessary to initialize ret, but it wasn't already\nso\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> -\tif (caps_.isMeta())\n> -\t\tret = trySetFormatMeta(format, true);\n> -\telse if (caps_.isMultiplanar())\n> -\t\tret = trySetFormatMultiplane(format, true);\n> -\telse\n> +\n> +\tswitch (bufferType_) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> \t\tret = trySetFormatSingleplane(format, true);\n> +\t\tbreak;\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> +\t\tret = trySetFormatMultiplane(format, true);\n> +\t\tbreak;\n> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> +\t\tret = trySetFormatMeta(format, true);\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tret = -EINVAL;\n> +\t\tbreak;\n> +\t}\n>\n> \t/* Cache the set format on success. */\n> \tif (ret)\n> --\n> 2.45.2\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 12568C323E\n\tfor ;\n\tTue, 16 Jul 2024 09:18:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB3106336F;\n\tTue, 16 Jul 2024 11:18:17 +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 19A09619A9\n\tfor ;\n\tTue, 16 Jul 2024 11:18:16 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:72c3:346:a663:c82d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C69E8229;\n\tTue, 16 Jul 2024 11:17:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cCniIJYf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721121458;\n\tbh=L+wbLnkzsrSaEJWmg7QI3V/AAyl8UFnRHqiYr0Zjpr4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cCniIJYfK5/7kQRE//MvQEK06EM2v4DypdoiW3Mm7megqSxKFy63IGpjVVzB3KAtq\n\tDi4sfxWZEcTsbVTB5VM+Nf+FkHZA2xSGYUM0wsxASKcEGC1DvF0NUVjzkbPeex45+o\n\tl57nHdK4OER0D2Hsxu8HKIi1uqnoDi6zcwEV4ga4=","Date":"Tue, 16 Jul 2024 11:18:12 +0200","From":"Jacopo Mondi ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, \n\tMilan Zamazal , Maxime Ripard ","Subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","Message-ID":"","References":"<20240713184327.23013-1-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240713184327.23013-1-hdegoede@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30412,"web_url":"https://patchwork.libcamera.org/comment/30412/","msgid":"<2fb88dd1-9877-4f7b-8b83-b8c40153abaf@redhat.com>","date":"2024-07-16T10:16:39","subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Jacopo,\n\nOn 7/16/24 11:18 AM, Jacopo Mondi wrote:\n> Hi Hans\n> \n> On Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote:\n>> V4L2VideoDevice is using the caps to determine which kind of buffers to\n>> use with the video-device in 2 different cases:\n>>\n>> 1. V4L2VideoDevice::open()\n>> 2. V4L2VideoDevice::[get|try|set]Format()\n>>\n>> And the order in which the caps are checked is different between\n>> these 2 cases. This is a problem for /dev/video# nodes which support\n>> both video-capture and metadata buffers. open() sets bufferType_ to\n>> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as\n>> [get|try|set]Format() will call [get|set]FormatMeta() which does not\n>> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.\n>>\n>> Switch [get|try|set]Format() to use the bufferType_ to determine on what\n>> sort of buffers they should be operating, leaving the V4L2VideoDevice\n>> code with only a single place where the decision is made what sort\n>> of buffers it should operate on for a specific /dev/video# node.\n>>\n>> This will also allow to modify open() in the future to take a bufferType\n>> argument to allow overriding the default bufferType it selects for\n>> /dev/video# nodes which are capable of supporting more then 1 buffer type.\n> \n> Does this mean that, at the moment, on a device that supports both\n> capture and metadata buffer types, video-capture is selected\n> unconditionally and the metadata interface cannot be used ?\n\nYes, this is an improvement over the current situation where\nneither works :)\n\nNote the metadata API's are still not entirely set in stone in\nthe kernel and are still disabled (behind an #ifdef) upstream atm.\nBut the IPU6 driver already advertises V4L2_CAP_META_CAPTURE. So\nfor now using video-capture instead of the not yet enabled metadata\nsupport is definitely the right thing to do.\n\n> If you plan to add an option to override the bufferType_ at open()\n> time, does it mean it is needed to go through a open-close-open cycle\n> to switch buffer type or:\n\nAt least the IPU6 has multiple dma engines represented by multiple\n/dev/video# nodes. So the idea would be to open one for video capture\nand one for metadata capture and then open both requesting capture\nbuffers while opening one and metadata buffers while opening the other.\n\nBut yes switching type on a single node would require a close + open,\nnote that changing type not only requires changing the bufferType_\nof V4L2VideoDevice if switching between output/capture it also\nrequires changing the fdBufferNotifier_ . So requiring a close()\n+ open() to switch type does not seem like a strange requirement.\n\n> 1) The device can be opened multiple times and bufferType_ gets\n> overriden ? (I'm not sure this is a good ideas as the last caller of\n> open() selects the bufferType_)\n\n/dev/video# nodes can be opened multiple times, but only to allow\nquerying caps / setting controls. Only one fd referring to it\ncan requestBuffers at a time. So basically from a libcamera pov\n/dev/video# nodes should not be opened multiple times.\n\n> 2) Should we instead allow overriding the bufferType_ to use with an\n> optional parameter to [get|try|set]Format() instead ?\n\nbufferType_ is also used internally by the V4L2VideoDevice code for\nenumPixelformats(), setSelection(), requestBuffers(), createBuffer(),\nexportDmabufFd(), etc.\n\nThe normal way of operating really is for there to be a fixed bufferType\nfor one specific /dev/video# nodes. Which is why I'm suggesting to\n(in the future when needed for metadata support) to add an optional\nbufferType argument to open(), which basically fixates the bufferType\nat open() time for a /dev/video# node which can support multiple types.\n\nRegards,\n\nHans\n\n\n\n\n> \n>>\n>> Signed-off-by: Hans de Goede \n>> ---\n>> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------\n>> 1 file changed, 41 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 4947aa3d..a8dc5355 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const\n>> */\n>> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n>> {\n>> -\tif (caps_.isMeta())\n>> -\t\treturn getFormatMeta(format);\n>> -\telse if (caps_.isMultiplanar())\n>> -\t\treturn getFormatMultiplane(format);\n>> -\telse\n>> +\tswitch (bufferType_) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> \t\treturn getFormatSingleplane(format);\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>> +\t\treturn getFormatMultiplane(format);\n>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>> +\t\treturn getFormatMeta(format);\n>> +\tdefault:\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> }\n>>\n>> /**\n>> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n>> */\n>> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n>> {\n>> -\tif (caps_.isMeta())\n>> -\t\treturn trySetFormatMeta(format, false);\n>> -\telse if (caps_.isMultiplanar())\n>> -\t\treturn trySetFormatMultiplane(format, false);\n>> -\telse\n>> +\tswitch (bufferType_) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> \t\treturn trySetFormatSingleplane(format, false);\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>> +\t\treturn trySetFormatMultiplane(format, false);\n>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>> +\t\treturn trySetFormatMeta(format, false);\n>> +\tdefault:\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> }\n>>\n>> /**\n>> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n>> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n>> {\n>> \tint ret = 0;\n> \n> I don't think it is necessary to initialize ret, but it wasn't already\n> so\n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n>> -\tif (caps_.isMeta())\n>> -\t\tret = trySetFormatMeta(format, true);\n>> -\telse if (caps_.isMultiplanar())\n>> -\t\tret = trySetFormatMultiplane(format, true);\n>> -\telse\n>> +\n>> +\tswitch (bufferType_) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> \t\tret = trySetFormatSingleplane(format, true);\n>> +\t\tbreak;\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>> +\t\tret = trySetFormatMultiplane(format, true);\n>> +\t\tbreak;\n>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>> +\t\tret = trySetFormatMeta(format, true);\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tret = -EINVAL;\n>> +\t\tbreak;\n>> +\t}\n>>\n>> \t/* Cache the set format on success. */\n>> \tif (ret)\n>> --\n>> 2.45.2\n>>\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 8B105BDB1C\n\tfor ;\n\tTue, 16 Jul 2024 10:16:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9881463369;\n\tTue, 16 Jul 2024 12:16:49 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5AD0E63365\n\tfor ;\n\tTue, 16 Jul 2024 12:16:47 +0200 (CEST)","from mail-lf1-f71.google.com (mail-lf1-f71.google.com\n\t[209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-501-dXu-zu0KP3GyqsItu9T1rg-1; Tue, 16 Jul 2024 06:16:43 -0400","by mail-lf1-f71.google.com with SMTP id\n\t2adb3069b0e04-52e969d0198so5583551e87.2\n\tfor ;\n\tTue, 16 Jul 2024 03:16:43 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a79bc820db8sm290851466b.213.2024.07.16.03.16.40\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 16 Jul 2024 03:16:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"jAebor24\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1721125006;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=P86X8DH1tjRDxXpwsQafgWt9fNbqvzEqoRJr7Z/GDvk=;\n\tb=jAebor24ifp3OX7SLllbGslkGynVxNKU8z5AxoAotRXShWTb4LD+KytIW5VcdgtEhZ54qy\n\tk3EbVbcO5j1oz0AJ/HgqF/IgacgMrUaXZLVE5gOFmzwHyI4RUNY9TPkKALX+Srtg9jR6LN\n\tFmIu5UKkR4Cw1WiFuZRn+h6nhYcKguw=","X-MC-Unique":"dXu-zu0KP3GyqsItu9T1rg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1721125002; x=1721729802;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=P86X8DH1tjRDxXpwsQafgWt9fNbqvzEqoRJr7Z/GDvk=;\n\tb=WgT0J18a+cPek0Me+TBWw+A2yzjJjG+3KUsmyMsBeXKrAquE5GUif0gY509Xjis4/g\n\tgw6G01a61jKwlXqBPkOZxsZTTbS7HPFxLUxQQAZTbTG0Q9biTx2KvwZztLhODgYNHkpF\n\tU/poUX3lHm9dA8WqVjtPgIlCmv+VbB709VOWpL3+l80IiYumOX0U7q44NzXpxzVAhPNq\n\tNshoqCoAsiPF/DuGGRlBsNoeKda5OdGjg+tkkqwrV0H8WFxFfLUn+2LESXp/U9EIcaaq\n\ts9HkA6XmgTTvWNk/BXT0O2gZsIW6ZgVad9Yk0n9uvTwJBt2CDSVK+lCtvUOjLtRex+9M\n\tGQxg==","X-Gm-Message-State":"AOJu0Yxkfor9q/6kUco/cdwr0UNxfYF6qM7ni7ESmkCXlSn30Luge5Fd\n\t8o9GOaxD9qXZGSTqqT4bMuBKHy6osRV0aEe96mm9siKW/ikQoxGLfV6SdKadY6rMPAObPsGuvlR\n\tfJUn2By4vHgW+Qmm5yMMX1cuR06+irdAha+hPHDWIgOCx0Dd+M1aKOnguFIDO/WE5HUvByAfEj/\n\t8A6to=","X-Received":["by 2002:a05:6512:ac2:b0:52c:e1cd:39b7 with SMTP id\n\t2adb3069b0e04-52edef0fdf1mr1282174e87.5.1721125001717; \n\tTue, 16 Jul 2024 03:16:41 -0700 (PDT)","by 2002:a05:6512:ac2:b0:52c:e1cd:39b7 with SMTP id\n\t2adb3069b0e04-52edef0fdf1mr1282157e87.5.1721125001250; \n\tTue, 16 Jul 2024 03:16:41 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEp5PT23B3vo3vC+yAof+Y5Cr++sVozGwUPcEAaRZEn+6h1cfqTv1+pk4jTC9X3lDRJwqaCbg==","Message-ID":"<2fb88dd1-9877-4f7b-8b83-b8c40153abaf@redhat.com>","Date":"Tue, 16 Jul 2024 12:16:39 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","References":"<20240713184327.23013-1-hdegoede@redhat.com>\n\t","From":"Hans de Goede ","In-Reply-To":"","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30414,"web_url":"https://patchwork.libcamera.org/comment/30414/","msgid":"<2tzgnpabz6l2ubouxsdc2ta6t3mocw74ims4bfmvsbjeqkxwqn@zqezmflsbl6p>","date":"2024-07-17T08:05:56","subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Hans\n\nOn Tue, Jul 16, 2024 at 12:16:39PM GMT, Hans de Goede wrote:\n> Hi Jacopo,\n>\n> On 7/16/24 11:18 AM, Jacopo Mondi wrote:\n> > Hi Hans\n> >\n> > On Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote:\n> >> V4L2VideoDevice is using the caps to determine which kind of buffers to\n> >> use with the video-device in 2 different cases:\n> >>\n> >> 1. V4L2VideoDevice::open()\n> >> 2. V4L2VideoDevice::[get|try|set]Format()\n> >>\n> >> And the order in which the caps are checked is different between\n> >> these 2 cases. This is a problem for /dev/video# nodes which support\n> >> both video-capture and metadata buffers. open() sets bufferType_ to\n> >> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as\n> >> [get|try|set]Format() will call [get|set]FormatMeta() which does not\n> >> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.\n> >>\n> >> Switch [get|try|set]Format() to use the bufferType_ to determine on what\n> >> sort of buffers they should be operating, leaving the V4L2VideoDevice\n> >> code with only a single place where the decision is made what sort\n> >> of buffers it should operate on for a specific /dev/video# node.\n> >>\n> >> This will also allow to modify open() in the future to take a bufferType\n> >> argument to allow overriding the default bufferType it selects for\n> >> /dev/video# nodes which are capable of supporting more then 1 buffer type.\n> >\n> > Does this mean that, at the moment, on a device that supports both\n> > capture and metadata buffer types, video-capture is selected\n> > unconditionally and the metadata interface cannot be used ?\n>\n> Yes, this is an improvement over the current situation where\n> neither works :)\n>\n> Note the metadata API's are still not entirely set in stone in\n> the kernel and are still disabled (behind an #ifdef) upstream atm.\n\nAh so we're not talking about the usual META_CAPTURE and META_OUTPUT\ninterfaces used to exchange ISP parameters, but we're here looking at\ncapturing sensor's produced metadata (which require streams and\nrouting support which are currently disabled behind an #ifdef)\n\n> But the IPU6 driver already advertises V4L2_CAP_META_CAPTURE. So\n> for now using video-capture instead of the not yet enabled metadata\n> support is definitely the right thing to do.\n>\n> > If you plan to add an option to override the bufferType_ at open()\n> > time, does it mean it is needed to go through a open-close-open cycle\n> > to switch buffer type or:\n>\n> At least the IPU6 has multiple dma engines represented by multiple\n> /dev/video# nodes. So the idea would be to open one for video capture\n> and one for metadata capture and then open both requesting capture\n> buffers while opening one and metadata buffers while opening the other.\n\nI think I got lost in the sequence of \"open\" and \"while opening\" but I\npresume you mean that a video node can be used either for\nvideo-capture and for meta-data capture, depending on the buffer type\nit is initialized with (I had a brief look at ipu6-isys-video.c and I\nsee the vb2 queue being initialized with the VIDEO_CAPTURE type\nunconditionally, I wonder how the META_CAPTURE interface will be\nselected in future, but that's a different topic).\n\n>\n> But yes switching type on a single node would require a close + open,\n> note that changing type not only requires changing the bufferType_\n> of V4L2VideoDevice if switching between output/capture it also\n> requires changing the fdBufferNotifier_ . So requiring a close()\n> + open() to switch type does not seem like a strange requirement.\n>\n> > 1) The device can be opened multiple times and bufferType_ gets\n> > overriden ? (I'm not sure this is a good ideas as the last caller of\n> > open() selects the bufferType_)\n>\n> /dev/video# nodes can be opened multiple times, but only to allow\n> querying caps / setting controls. Only one fd referring to it\n> can requestBuffers at a time. So basically from a libcamera pov\n> /dev/video# nodes should not be opened multiple times.\n>\n\nok\n\n> > 2) Should we instead allow overriding the bufferType_ to use with an\n> > optional parameter to [get|try|set]Format() instead ?\n>\n> bufferType_ is also used internally by the V4L2VideoDevice code for\n> enumPixelformats(), setSelection(), requestBuffers(), createBuffer(),\n> exportDmabufFd(), etc.\n>\n> The normal way of operating really is for there to be a fixed bufferType\n> for one specific /dev/video# nodes. Which is why I'm suggesting to\n> (in the future when needed for metadata support) to add an optional\n> bufferType argument to open(), which basically fixates the bufferType\n> at open() time for a /dev/video# node which can support multiple types.\n\nWe should probably make sure then that if a video device is 'forced'\nto use a buffer type at open() time, all the follwing open() call\nwon't force it to a different buffer type ? But this is indeed for\nlater.\n\nThanks\n j\n\n>\n> Regards,\n>\n> Hans\n>\n>\n>\n>\n> >\n> >>\n> >> Signed-off-by: Hans de Goede \n> >> ---\n> >> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------\n> >> 1 file changed, 41 insertions(+), 15 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >> index 4947aa3d..a8dc5355 100644\n> >> --- a/src/libcamera/v4l2_videodevice.cpp\n> >> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const\n> >> */\n> >> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> >> {\n> >> -\tif (caps_.isMeta())\n> >> -\t\treturn getFormatMeta(format);\n> >> -\telse if (caps_.isMultiplanar())\n> >> -\t\treturn getFormatMultiplane(format);\n> >> -\telse\n> >> +\tswitch (bufferType_) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> \t\treturn getFormatSingleplane(format);\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> >> +\t\treturn getFormatMultiplane(format);\n> >> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> >> +\t\treturn getFormatMeta(format);\n> >> +\tdefault:\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> }\n> >>\n> >> /**\n> >> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> >> */\n> >> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> >> {\n> >> -\tif (caps_.isMeta())\n> >> -\t\treturn trySetFormatMeta(format, false);\n> >> -\telse if (caps_.isMultiplanar())\n> >> -\t\treturn trySetFormatMultiplane(format, false);\n> >> -\telse\n> >> +\tswitch (bufferType_) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> \t\treturn trySetFormatSingleplane(format, false);\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> >> +\t\treturn trySetFormatMultiplane(format, false);\n> >> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> >> +\t\treturn trySetFormatMeta(format, false);\n> >> +\tdefault:\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> }\n> >>\n> >> /**\n> >> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> >> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> >> {\n> >> \tint ret = 0;\n> >\n> > I don't think it is necessary to initialize ret, but it wasn't already\n> > so\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> >> -\tif (caps_.isMeta())\n> >> -\t\tret = trySetFormatMeta(format, true);\n> >> -\telse if (caps_.isMultiplanar())\n> >> -\t\tret = trySetFormatMultiplane(format, true);\n> >> -\telse\n> >> +\n> >> +\tswitch (bufferType_) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> \t\tret = trySetFormatSingleplane(format, true);\n> >> +\t\tbreak;\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> >> +\t\tret = trySetFormatMultiplane(format, true);\n> >> +\t\tbreak;\n> >> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> >> +\t\tret = trySetFormatMeta(format, true);\n> >> +\t\tbreak;\n> >> +\tdefault:\n> >> +\t\tret = -EINVAL;\n> >> +\t\tbreak;\n> >> +\t}\n> >>\n> >> \t/* Cache the set format on success. */\n> >> \tif (ret)\n> >> --\n> >> 2.45.2\n> >>\n> >\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 CCAFBBDB1C\n\tfor ;\n\tWed, 17 Jul 2024 08:06:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B23FE6336F;\n\tWed, 17 Jul 2024 10:06:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64882619A7\n\tfor ;\n\tWed, 17 Jul 2024 10:05:59 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 51464836;\n\tWed, 17 Jul 2024 10:05:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uUnsgwzW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721203521;\n\tbh=oiAwotaa82vjwmH9w/oPdEfxcSEgRfkwF5d0/pVZmV8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uUnsgwzWiCBnQH+DZhPNBkWj6zqAkCG/o5oC3QB8LT4NCpMmo6PpcGlfEd9UJdyra\n\tEUhp4WuQdyl9m2YV9JbZ4dEmrjm0EnpRseGMebrNcNNHtlKtUdAF1Jhr6MfPManZlf\n\tciu4FlqcPRiJb3dpOATTBcs2eE78euMNhg9eTy20=","Date":"Wed, 17 Jul 2024 10:05:56 +0200","From":"Jacopo Mondi ","To":"Hans de Goede ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , Maxime Ripard ","Subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","Message-ID":"<2tzgnpabz6l2ubouxsdc2ta6t3mocw74ims4bfmvsbjeqkxwqn@zqezmflsbl6p>","References":"<20240713184327.23013-1-hdegoede@redhat.com>\n\t\n\t<2fb88dd1-9877-4f7b-8b83-b8c40153abaf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2fb88dd1-9877-4f7b-8b83-b8c40153abaf@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30437,"web_url":"https://patchwork.libcamera.org/comment/30437/","msgid":"<20240719073700.GC12656@pendragon.ideasonboard.com>","date":"2024-07-19T07:37:00","subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nThank you for the patch.\n\nOn Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote:\n> V4L2VideoDevice is using the caps to determine which kind of buffers to\n> use with the video-device in 2 different cases:\n> \n> 1. V4L2VideoDevice::open()\n> 2. V4L2VideoDevice::[get|try|set]Format()\n> \n> And the order in which the caps are checked is different between\n> these 2 cases. This is a problem for /dev/video# nodes which support\n> both video-capture and metadata buffers. open() sets bufferType_ to\n> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as\n> [get|try|set]Format() will call [get|set]FormatMeta() which does not\n> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.\n> \n> Switch [get|try|set]Format() to use the bufferType_ to determine on what\n> sort of buffers they should be operating, leaving the V4L2VideoDevice\n> code with only a single place where the decision is made what sort\n> of buffers it should operate on for a specific /dev/video# node.\n> \n> This will also allow to modify open() in the future to take a bufferType\n> argument to allow overriding the default bufferType it selects for\n> /dev/video# nodes which are capable of supporting more then 1 buffer type.\n> \n> Signed-off-by: Hans de Goede \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------\n> 1 file changed, 41 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 4947aa3d..a8dc5355 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const\n> */\n> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> {\n> -\tif (caps_.isMeta())\n> -\t\treturn getFormatMeta(format);\n> -\telse if (caps_.isMultiplanar())\n> -\t\treturn getFormatMultiplane(format);\n> -\telse\n> +\tswitch (bufferType_) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> \t\treturn getFormatSingleplane(format);\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> +\t\treturn getFormatMultiplane(format);\n> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> +\t\treturn getFormatMeta(format);\n> +\tdefault:\n> +\t\treturn -EINVAL;\n\nThis can never happen, so you could fold the default vase with any of\nthe above. I suppose returning -EINVAL could help catching future errors\nwhen we'll update open() to support more types but forget to update the\ncode here. Is that a real risk ?\n\n> +\t}\n> }\n> \n> /**\n> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> */\n> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> {\n> -\tif (caps_.isMeta())\n> -\t\treturn trySetFormatMeta(format, false);\n> -\telse if (caps_.isMultiplanar())\n> -\t\treturn trySetFormatMultiplane(format, false);\n> -\telse\n> +\tswitch (bufferType_) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> \t\treturn trySetFormatSingleplane(format, false);\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> +\t\treturn trySetFormatMultiplane(format, false);\n> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> +\t\treturn trySetFormatMeta(format, false);\n> +\tdefault:\n> +\t\treturn -EINVAL;\n\nSame here.\n\n> +\t}\n> }\n> \n> /**\n> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> {\n> \tint ret = 0;\n\nWhile at it you can drop the initialization of the ret variable.\n\n> -\tif (caps_.isMeta())\n> -\t\tret = trySetFormatMeta(format, true);\n> -\telse if (caps_.isMultiplanar())\n> -\t\tret = trySetFormatMultiplane(format, true);\n> -\telse\n> +\n> +\tswitch (bufferType_) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> \t\tret = trySetFormatSingleplane(format, true);\n> +\t\tbreak;\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> +\t\tret = trySetFormatMultiplane(format, true);\n> +\t\tbreak;\n> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> +\t\tret = trySetFormatMeta(format, true);\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tret = -EINVAL;\n> +\t\tbreak;\n\nAnd here.\n\nIf you think we should drop the default case, please send a new version.\nOtherwise I can drop the initialization of ret when applying. Either\nway,\n\nReviewed-by: Laurent Pinchart \n\n> +\t}\n> \n> \t/* Cache the set format on success. */\n> \tif (ret)","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 07160BDB1C\n\tfor ;\n\tFri, 19 Jul 2024 07:37:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D501B6336F;\n\tFri, 19 Jul 2024 09:37:17 +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 5507B619A3\n\tfor ;\n\tFri, 19 Jul 2024 09:37:15 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EBD4E73E;\n\tFri, 19 Jul 2024 09:36:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sscrirkp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721374596;\n\tbh=SIWPn+Py2Njszx5r4LttpBLTI0PMDDZLoD80+Xx6Hgk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sscrirkp8Nm4+QSuFOPhIIEPshtFASu3aLxQWTUO+sUL+9F5VnxW+8OKyYNyjFHUp\n\tyMRU1+eS6VB2EAVHcGZKD/7OtK2BhGp8PdoYLKqwMMAWQJU/KNnp1biv1PZ6/4GQ+q\n\tzUwHdnTl98bXt87qVZ16uWD4El0Eo9HUZmCb4xVg=","Date":"Fri, 19 Jul 2024 10:37:00 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","Subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","Message-ID":"<20240719073700.GC12656@pendragon.ideasonboard.com>","References":"<20240713184327.23013-1-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240713184327.23013-1-hdegoede@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30440,"web_url":"https://patchwork.libcamera.org/comment/30440/","msgid":"<9ba20694-cbc1-41e6-b6e6-530dc68fd590@redhat.com>","date":"2024-07-19T13:07:26","subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Laurent,\n\nOn 7/19/24 9:37 AM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> Thank you for the patch.\n> \n> On Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote:\n>> V4L2VideoDevice is using the caps to determine which kind of buffers to\n>> use with the video-device in 2 different cases:\n>>\n>> 1. V4L2VideoDevice::open()\n>> 2. V4L2VideoDevice::[get|try|set]Format()\n>>\n>> And the order in which the caps are checked is different between\n>> these 2 cases. This is a problem for /dev/video# nodes which support\n>> both video-capture and metadata buffers. open() sets bufferType_ to\n>> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as\n>> [get|try|set]Format() will call [get|set]FormatMeta() which does not\n>> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.\n>>\n>> Switch [get|try|set]Format() to use the bufferType_ to determine on what\n>> sort of buffers they should be operating, leaving the V4L2VideoDevice\n>> code with only a single place where the decision is made what sort\n>> of buffers it should operate on for a specific /dev/video# node.\n>>\n>> This will also allow to modify open() in the future to take a bufferType\n>> argument to allow overriding the default bufferType it selects for\n>> /dev/video# nodes which are capable of supporting more then 1 buffer type.\n>>\n>> Signed-off-by: Hans de Goede \n>> ---\n>> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------\n>> 1 file changed, 41 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 4947aa3d..a8dc5355 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const\n>> */\n>> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n>> {\n>> -\tif (caps_.isMeta())\n>> -\t\treturn getFormatMeta(format);\n>> -\telse if (caps_.isMultiplanar())\n>> -\t\treturn getFormatMultiplane(format);\n>> -\telse\n>> +\tswitch (bufferType_) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> \t\treturn getFormatSingleplane(format);\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>> +\t\treturn getFormatMultiplane(format);\n>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>> +\t\treturn getFormatMeta(format);\n>> +\tdefault:\n>> +\t\treturn -EINVAL;\n> \n> This can never happen, so you could fold the default vase with any of\n> the above. I suppose returning -EINVAL could help catching future errors\n> when we'll update open() to support more types.\n\nRight I'm not a fan of just adding a default: to some existing case\nwhen there is no clear default existing case.\n\n> but forget to update the\n> code here. Is that a real risk ?\n\nHumans make mistakes so yes IMHO this is real risk and I would\nprefer to keep this bit as is.\n\n> \n>> +\t}\n>> }\n>> \n>> /**\n>> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n>> */\n>> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n>> {\n>> -\tif (caps_.isMeta())\n>> -\t\treturn trySetFormatMeta(format, false);\n>> -\telse if (caps_.isMultiplanar())\n>> -\t\treturn trySetFormatMultiplane(format, false);\n>> -\telse\n>> +\tswitch (bufferType_) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> \t\treturn trySetFormatSingleplane(format, false);\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>> +\t\treturn trySetFormatMultiplane(format, false);\n>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>> +\t\treturn trySetFormatMeta(format, false);\n>> +\tdefault:\n>> +\t\treturn -EINVAL;\n> \n> Same here.\n> \n>> +\t}\n>> }\n>> \n>> /**\n>> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n>> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n>> {\n>> \tint ret = 0;\n> \n> While at it you can drop the initialization of the ret variable.\n\nAck.\n\n> \n>> -\tif (caps_.isMeta())\n>> -\t\tret = trySetFormatMeta(format, true);\n>> -\telse if (caps_.isMultiplanar())\n>> -\t\tret = trySetFormatMultiplane(format, true);\n>> -\telse\n>> +\n>> +\tswitch (bufferType_) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> \t\tret = trySetFormatSingleplane(format, true);\n>> +\t\tbreak;\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>> +\t\tret = trySetFormatMultiplane(format, true);\n>> +\t\tbreak;\n>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>> +\t\tret = trySetFormatMeta(format, true);\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tret = -EINVAL;\n>> +\t\tbreak;\n> \n> And here.\n> \n> If you think we should drop the default case, please send a new version.\n> Otherwise I can drop the initialization of ret when applying. Either\n> way,\n\nAs mendtioned above I prefer to keep the default case. If you can\ndrop the initialization of ret while applying this that would be\ngreat.\n\n> Reviewed-by: Laurent Pinchart \n\nThanks & Regsrds,\n\nHans\n\n\n\n\n\n>> \n>> \t/* Cache the set format on success. */\n>> \tif (ret)\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 F036BC323E\n\tfor ;\n\tFri, 19 Jul 2024 13:07:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6C2E6336F;\n\tFri, 19 Jul 2024 15:07:36 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C988619A7\n\tfor ;\n\tFri, 19 Jul 2024 15:07:34 +0200 (CEST)","from mail-lf1-f70.google.com (mail-lf1-f70.google.com\n\t[209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-549-CyBRbJYMMWyeqgCNoOzOqg-1; Fri, 19 Jul 2024 09:07:30 -0400","by mail-lf1-f70.google.com with SMTP id\n\t2adb3069b0e04-52e960523ecso1356783e87.1\n\tfor ;\n\tFri, 19 Jul 2024 06:07:30 -0700 (PDT)","from [192.168.2.152] ([90.187.152.45])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a7a3c7be6f8sm31204466b.76.2024.07.19.06.07.27\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 19 Jul 2024 06:07:27 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"ZTo6Smk2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1721394453;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=z522D3s8h0YZfZLarn/tZVIwU4qM6X2RYk9+J8z0jxk=;\n\tb=ZTo6Smk2sJ6FJTDSPEuHbhrsaTMJVtUc2rOOUsDZnxhDNfoqGFlOCkAM6lhk0wn4jFtOqb\n\t7YS/lnf6YJj0l8an32Ch8FYEFmUN7i48Da9mrihDuX09m/93OXeqEcTNzYWCBB61SBn214\n\tvZj0IB9ZZIcDPWuzjamxai0YglWYf5I=","X-MC-Unique":"CyBRbJYMMWyeqgCNoOzOqg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1721394449; x=1721999249;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=z522D3s8h0YZfZLarn/tZVIwU4qM6X2RYk9+J8z0jxk=;\n\tb=jU/exLEM2MdDKGVbogWO4kQ4PIEJeyyk/C/d5eEb9neyMefllQezBAWP8JCn2p7sUe\n\tbhw6CjYW+DoaU7a5u7C7DfdljjJHxAhoa+QRoW9mRJ+74ZkrQRu1RTEwOjT7pla9jgBi\n\twa8bZfXWQelVR1bQ7n3uHNl7RYGhSHJk0lenVbFReMh8vY4RtCtep2blLHj1Xit6SUMS\n\trjoJIWGIuyJgg5+822jud33XakBZOoBXSnOCunQjl7hxqAd47fD63PBg96wEIAKaU3n8\n\tLtVvQUkTDf9WONtfjSgNwezEi1XvxN/vk7KxPNpUFWSXC6hSaekDzyyv7DpSG+yEDy6F\n\t/VGQ==","X-Gm-Message-State":"AOJu0Yw0Lhnn40Vc3ivIHc/DjODRLMdnPsgpiYUbFAXzumRtqXyfceb9\n\tNj3NttdBLAGytA1VvB/mkSQiiW7YA8SwU582bTGxuW+WXx8JQom/Dp0W+WcOM57il1Arf7hBKUz\n\tcqdkbXbpui9w48UqkNS15Xcn6Ch/nFwdZuGSczheL1NBckNIp7xs3g5bvOZ3oqPRHjdeimHI=","X-Received":["by 2002:a05:6512:e83:b0:52e:9b4f:dd8c with SMTP id\n\t2adb3069b0e04-52ef57511b4mr1162336e87.35.1721394449173; \n\tFri, 19 Jul 2024 06:07:29 -0700 (PDT)","by 2002:a05:6512:e83:b0:52e:9b4f:dd8c with SMTP id\n\t2adb3069b0e04-52ef57511b4mr1162318e87.35.1721394448745; \n\tFri, 19 Jul 2024 06:07:28 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH0VCRx+ne2RDH2UCkCFaGhN9rtGPNCoRK1LLofWW4oZUKaJDZuo6P1J7exm5/9hVEjoqi+OA==","Message-ID":"<9ba20694-cbc1-41e6-b6e6-530dc68fd590@redhat.com>","Date":"Fri, 19 Jul 2024 15:07:26 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","References":"<20240713184327.23013-1-hdegoede@redhat.com>\n\t<20240719073700.GC12656@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240719073700.GC12656@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30441,"web_url":"https://patchwork.libcamera.org/comment/30441/","msgid":"<20240721163305.GA29895@pendragon.ideasonboard.com>","date":"2024-07-21T16:33:05","subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Fri, Jul 19, 2024 at 03:07:26PM +0200, Hans de Goede wrote:\n> On 7/19/24 9:37 AM, Laurent Pinchart wrote:\n> > On Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote:\n> >> V4L2VideoDevice is using the caps to determine which kind of buffers to\n> >> use with the video-device in 2 different cases:\n> >>\n> >> 1. V4L2VideoDevice::open()\n> >> 2. V4L2VideoDevice::[get|try|set]Format()\n> >>\n> >> And the order in which the caps are checked is different between\n> >> these 2 cases. This is a problem for /dev/video# nodes which support\n> >> both video-capture and metadata buffers. open() sets bufferType_ to\n> >> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as\n> >> [get|try|set]Format() will call [get|set]FormatMeta() which does not\n> >> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.\n> >>\n> >> Switch [get|try|set]Format() to use the bufferType_ to determine on what\n> >> sort of buffers they should be operating, leaving the V4L2VideoDevice\n> >> code with only a single place where the decision is made what sort\n> >> of buffers it should operate on for a specific /dev/video# node.\n> >>\n> >> This will also allow to modify open() in the future to take a bufferType\n> >> argument to allow overriding the default bufferType it selects for\n> >> /dev/video# nodes which are capable of supporting more then 1 buffer type.\n> >>\n> >> Signed-off-by: Hans de Goede \n> >> ---\n> >> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------\n> >> 1 file changed, 41 insertions(+), 15 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >> index 4947aa3d..a8dc5355 100644\n> >> --- a/src/libcamera/v4l2_videodevice.cpp\n> >> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const\n> >> */\n> >> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> >> {\n> >> -\tif (caps_.isMeta())\n> >> -\t\treturn getFormatMeta(format);\n> >> -\telse if (caps_.isMultiplanar())\n> >> -\t\treturn getFormatMultiplane(format);\n> >> -\telse\n> >> +\tswitch (bufferType_) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> \t\treturn getFormatSingleplane(format);\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> >> +\t\treturn getFormatMultiplane(format);\n> >> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> >> +\t\treturn getFormatMeta(format);\n> >> +\tdefault:\n> >> +\t\treturn -EINVAL;\n> > \n> > This can never happen, so you could fold the default vase with any of\n> > the above. I suppose returning -EINVAL could help catching future errors\n> > when we'll update open() to support more types.\n> \n> Right I'm not a fan of just adding a default: to some existing case\n> when there is no clear default existing case.\n> \n> > but forget to update the\n> > code here. Is that a real risk ?\n> \n> Humans make mistakes so yes IMHO this is real risk and I would\n> prefer to keep this bit as is.\n\nOK.\n\n> >> +\t}\n> >> }\n> >> \n> >> /**\n> >> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> >> */\n> >> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> >> {\n> >> -\tif (caps_.isMeta())\n> >> -\t\treturn trySetFormatMeta(format, false);\n> >> -\telse if (caps_.isMultiplanar())\n> >> -\t\treturn trySetFormatMultiplane(format, false);\n> >> -\telse\n> >> +\tswitch (bufferType_) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> \t\treturn trySetFormatSingleplane(format, false);\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> >> +\t\treturn trySetFormatMultiplane(format, false);\n> >> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> >> +\t\treturn trySetFormatMeta(format, false);\n> >> +\tdefault:\n> >> +\t\treturn -EINVAL;\n> > \n> > Same here.\n> > \n> >> +\t}\n> >> }\n> >> \n> >> /**\n> >> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> >> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> >> {\n> >> \tint ret = 0;\n> > \n> > While at it you can drop the initialization of the ret variable.\n> \n> Ack.\n> \n> >> -\tif (caps_.isMeta())\n> >> -\t\tret = trySetFormatMeta(format, true);\n> >> -\telse if (caps_.isMultiplanar())\n> >> -\t\tret = trySetFormatMultiplane(format, true);\n> >> -\telse\n> >> +\n> >> +\tswitch (bufferType_) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> \t\tret = trySetFormatSingleplane(format, true);\n> >> +\t\tbreak;\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> >> +\t\tret = trySetFormatMultiplane(format, true);\n> >> +\t\tbreak;\n> >> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n> >> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n> >> +\t\tret = trySetFormatMeta(format, true);\n> >> +\t\tbreak;\n> >> +\tdefault:\n> >> +\t\tret = -EINVAL;\n> >> +\t\tbreak;\n> > \n> > And here.\n> > \n> > If you think we should drop the default case, please send a new version.\n> > Otherwise I can drop the initialization of ret when applying. Either\n> > way,\n> \n> As mendtioned above I prefer to keep the default case. If you can\n> drop the initialization of ret while applying this that would be\n> great.\n> \n> > Reviewed-by: Laurent Pinchart \n> \n> Thanks & Regsrds,\n\nPushed. Unless I'm mistaken, initial IPU6 support is now fully merged.\n\n> >> \n> >> \t/* Cache the set format on success. */\n> >> \tif (ret)","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 6224BBDB1C\n\tfor ;\n\tSun, 21 Jul 2024 16:33:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 511086336F;\n\tSun, 21 Jul 2024 18:33:25 +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 1E6A56336B\n\tfor ;\n\tSun, 21 Jul 2024 18:33:23 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09469220;\n\tSun, 21 Jul 2024 18:32:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LIs76Tzd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721579562;\n\tbh=PEJMWPt+/xTCscC2Qj8Cdy2xWOyECwTTCMzFZaDyf0M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LIs76Tzdd58YM+BvuLTZiMDTrDXOgWUcorVRiywmZhAVfEEOAEqwxVn4pXjywjh3F\n\twsv+udzsHcExpSb9/BEPykCFuy7rtECXdMD0YLBh3SSHZwAhZirGUi65+tjmdFBg+f\n\tlAABxqtuci83HsxJbn7soe6dSEBoG9OHhXQ0VbLE=","Date":"Sun, 21 Jul 2024 19:33:05 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","Subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","Message-ID":"<20240721163305.GA29895@pendragon.ideasonboard.com>","References":"<20240713184327.23013-1-hdegoede@redhat.com>\n\t<20240719073700.GC12656@pendragon.ideasonboard.com>\n\t<9ba20694-cbc1-41e6-b6e6-530dc68fd590@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<9ba20694-cbc1-41e6-b6e6-530dc68fd590@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30442,"web_url":"https://patchwork.libcamera.org/comment/30442/","msgid":"<1f7bbaaf-8ae7-430f-aaaa-e906eb77efbf@redhat.com>","date":"2024-07-21T16:47:17","subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 7/21/24 6:33 PM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> On Fri, Jul 19, 2024 at 03:07:26PM +0200, Hans de Goede wrote:\n>> On 7/19/24 9:37 AM, Laurent Pinchart wrote:\n>>> On Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote:\n>>>> V4L2VideoDevice is using the caps to determine which kind of buffers to\n>>>> use with the video-device in 2 different cases:\n>>>>\n>>>> 1. V4L2VideoDevice::open()\n>>>> 2. V4L2VideoDevice::[get|try|set]Format()\n>>>>\n>>>> And the order in which the caps are checked is different between\n>>>> these 2 cases. This is a problem for /dev/video# nodes which support\n>>>> both video-capture and metadata buffers. open() sets bufferType_ to\n>>>> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as\n>>>> [get|try|set]Format() will call [get|set]FormatMeta() which does not\n>>>> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.\n>>>>\n>>>> Switch [get|try|set]Format() to use the bufferType_ to determine on what\n>>>> sort of buffers they should be operating, leaving the V4L2VideoDevice\n>>>> code with only a single place where the decision is made what sort\n>>>> of buffers it should operate on for a specific /dev/video# node.\n>>>>\n>>>> This will also allow to modify open() in the future to take a bufferType\n>>>> argument to allow overriding the default bufferType it selects for\n>>>> /dev/video# nodes which are capable of supporting more then 1 buffer type.\n>>>>\n>>>> Signed-off-by: Hans de Goede \n>>>> ---\n>>>> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------\n>>>> 1 file changed, 41 insertions(+), 15 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>>> index 4947aa3d..a8dc5355 100644\n>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>>> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const\n>>>> */\n>>>> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n>>>> {\n>>>> -\tif (caps_.isMeta())\n>>>> -\t\treturn getFormatMeta(format);\n>>>> -\telse if (caps_.isMultiplanar())\n>>>> -\t\treturn getFormatMultiplane(format);\n>>>> -\telse\n>>>> +\tswitch (bufferType_) {\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>>>> \t\treturn getFormatSingleplane(format);\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>>>> +\t\treturn getFormatMultiplane(format);\n>>>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>>>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>>>> +\t\treturn getFormatMeta(format);\n>>>> +\tdefault:\n>>>> +\t\treturn -EINVAL;\n>>>\n>>> This can never happen, so you could fold the default vase with any of\n>>> the above. I suppose returning -EINVAL could help catching future errors\n>>> when we'll update open() to support more types.\n>>\n>> Right I'm not a fan of just adding a default: to some existing case\n>> when there is no clear default existing case.\n>>\n>>> but forget to update the\n>>> code here. Is that a real risk ?\n>>\n>> Humans make mistakes so yes IMHO this is real risk and I would\n>> prefer to keep this bit as is.\n> \n> OK.\n> \n>>>> +\t}\n>>>> }\n>>>> \n>>>> /**\n>>>> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n>>>> */\n>>>> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n>>>> {\n>>>> -\tif (caps_.isMeta())\n>>>> -\t\treturn trySetFormatMeta(format, false);\n>>>> -\telse if (caps_.isMultiplanar())\n>>>> -\t\treturn trySetFormatMultiplane(format, false);\n>>>> -\telse\n>>>> +\tswitch (bufferType_) {\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>>>> \t\treturn trySetFormatSingleplane(format, false);\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>>>> +\t\treturn trySetFormatMultiplane(format, false);\n>>>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>>>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>>>> +\t\treturn trySetFormatMeta(format, false);\n>>>> +\tdefault:\n>>>> +\t\treturn -EINVAL;\n>>>\n>>> Same here.\n>>>\n>>>> +\t}\n>>>> }\n>>>> \n>>>> /**\n>>>> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n>>>> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n>>>> {\n>>>> \tint ret = 0;\n>>>\n>>> While at it you can drop the initialization of the ret variable.\n>>\n>> Ack.\n>>\n>>>> -\tif (caps_.isMeta())\n>>>> -\t\tret = trySetFormatMeta(format, true);\n>>>> -\telse if (caps_.isMultiplanar())\n>>>> -\t\tret = trySetFormatMultiplane(format, true);\n>>>> -\telse\n>>>> +\n>>>> +\tswitch (bufferType_) {\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>>>> \t\tret = trySetFormatSingleplane(format, true);\n>>>> +\t\tbreak;\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>>>> +\t\tret = trySetFormatMultiplane(format, true);\n>>>> +\t\tbreak;\n>>>> +\tcase V4L2_BUF_TYPE_META_CAPTURE:\n>>>> +\tcase V4L2_BUF_TYPE_META_OUTPUT:\n>>>> +\t\tret = trySetFormatMeta(format, true);\n>>>> +\t\tbreak;\n>>>> +\tdefault:\n>>>> +\t\tret = -EINVAL;\n>>>> +\t\tbreak;\n>>>\n>>> And here.\n>>>\n>>> If you think we should drop the default case, please send a new version.\n>>> Otherwise I can drop the initialization of ret when applying. Either\n>>> way,\n>>\n>> As mendtioned above I prefer to keep the default case. If you can\n>> drop the initialization of ret while applying this that would be\n>> great.\n>>\n>>> Reviewed-by: Laurent Pinchart \n>>\n>> Thanks & Regsrds,\n> \n> Pushed.\n\nThank you.\n\n> Unless I'm mistaken, initial IPU6 support is now fully merged.\n\nCorrect \\o/\n\nRegards,\n\nHans","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 85B82C323E\n\tfor ;\n\tSun, 21 Jul 2024 16:47:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 943206336F;\n\tSun, 21 Jul 2024 18:47:26 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB74B6336B\n\tfor ;\n\tSun, 21 Jul 2024 18:47:24 +0200 (CEST)","from mail-lj1-f197.google.com (mail-lj1-f197.google.com\n\t[209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-1-wUibhG5ROIaZPYKAzNWpXQ-1; Sun, 21 Jul 2024 12:47:21 -0400","by mail-lj1-f197.google.com with SMTP id\n\t38308e7fff4ca-2ef233fb86bso15362541fa.1\n\tfor ;\n\tSun, 21 Jul 2024 09:47:21 -0700 (PDT)","from [192.168.2.132] (business-90-187-152-45.pool2.vodafone-ip.de.\n\t[90.187.152.45]) by smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a7a3c7bda4csm309327766b.59.2024.07.21.09.47.18\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 21 Jul 2024 09:47:19 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"fBejoVW9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1721580443;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=+IL24aeKuDnbCQDqQqUR+YgmFHc6c6tQfJl+pvqpOJo=;\n\tb=fBejoVW943Aw/kUntM5BZVGWqG8LuhhwctTR5MeqyCzuO7ordVLCRG9RYy5Hvj1KPBSBlv\n\tCsrzLeBg8Podd3JkiEMtNhH2olYpvQufUCdqsvMauDuWR9b5sBla9WU6SCbc8usUhpYOGB\n\t/H9m7WN/0qhf/g+lCLbhuVM62iUMQSM=","X-MC-Unique":"wUibhG5ROIaZPYKAzNWpXQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1721580440; x=1722185240;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=+IL24aeKuDnbCQDqQqUR+YgmFHc6c6tQfJl+pvqpOJo=;\n\tb=uMKAZJTkLlWAFnB64IZtDjw0nirayorZCh+dfDKZJtpCGp94pPAyEzQkBRgdc7Op5W\n\tXrWdguAg7lAIN5k4pSrG06eU4Qif35WIS6Fk4ME26xJ6kwkFdF2q1mucS5MLJSLl/pJJ\n\tPolk0FgexDxuYAaYlBI3/+5iH4SRyxlYfL1bIrtTqGtSSU2X2ErQjRYaZmIY6CviZ2Wr\n\tlPNVFDuURE/xQ2zjtUwUKWvBr/eJG9m+Q/5KmGLDbp+qceGXEYl7J7x8NFbjMMcKlq0U\n\tOna181gR8u1XaPTt8vRR1OR0XeqJLXLsRNWBKc23xpMl1wz90QU/hepw+xHLH+2jX+f9\n\tRaOA==","X-Gm-Message-State":"AOJu0YzEerkeLeKbhBvZmfhel/iI0s9V4AnfhDlu2K70EV1lerHufeee\n\tctdlXCwZO1n+6L1+fG0zwXvQHXswJbqfxREW5ZtlnyccTcNah791j5zmGc7coqVxm/5sEBqHuWI\n\tFvVcLvLbYB3UxKOfY4KqZvZ46ojoBhm2gWrMt0PSf1Vq36JFK9aXKZmAKvs6EU3siLoPqk/Vw/A\n\t6ZS7k=","X-Received":["by 2002:a2e:9e86:0:b0:2ee:7d6d:2ce3 with SMTP id\n\t38308e7fff4ca-2ef167b4ba8mr43564951fa.13.1721580440070; \n\tSun, 21 Jul 2024 09:47:20 -0700 (PDT)","by 2002:a2e:9e86:0:b0:2ee:7d6d:2ce3 with SMTP id\n\t38308e7fff4ca-2ef167b4ba8mr43564881fa.13.1721580439633; \n\tSun, 21 Jul 2024 09:47:19 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHtjLdy/nmWVH6KVGjrdY1jfB8hKrpYt5UdPsseOYpcH2PyvtQLy6kXKbCie/BnHNQLZsc/Qg==","Message-ID":"<1f7bbaaf-8ae7-430f-aaaa-e906eb77efbf@redhat.com>","Date":"Sun, 21 Jul 2024 18:47:17 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: v4l2_videodevice: Use bufferType_ in\n\t[get|try|set]Format()","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","References":"<20240713184327.23013-1-hdegoede@redhat.com>\n\t<20240719073700.GC12656@pendragon.ideasonboard.com>\n\t<9ba20694-cbc1-41e6-b6e6-530dc68fd590@redhat.com>\n\t<20240721163305.GA29895@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240721163305.GA29895@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]