[{"id":27418,"web_url":"https://patchwork.libcamera.org/comment/27418/","msgid":"<20230623234847.GC1669@pendragon.ideasonboard.com>","date":"2023-06-23T23:48:47","subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nCC'ing Naush, as a review from Raspberry Pi would be nice.\n\nOn Thu, Jun 22, 2023 at 08:30:15PM +0530, Umang Jain via libcamera-devel wrote:\n> From: Lee Jackson \n> \n> Adds the PDAF support for IMX519 camera module by Arducam.\n> \n> Tested-by: Umang Jain \n> Signed-off-by: Umang Jain \n> Signed-off-by: Lee Jackson \n> ---\n> Changes in v3:\n> - Collect author's S-o-B from v2.\n> \n> ---\n> src/ipa/rpi/cam_helper/cam_helper_imx519.cpp | 50 ++++++++++++++++++++\n> src/ipa/rpi/vc4/data/imx519.json | 40 ++++++++++++++++\n> 2 files changed, 90 insertions(+)\n> \n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> index c7262aa0..6d032082 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> @@ -15,9 +15,13 @@\n> \n> #include \n> \n> +#include \"controller/pdaf_data.h\"\n> +\n> #include \"cam_helper.h\"\n> #include \"md_parser.h\"\n> \n> +#define ALIGN_UP(x, a) (((x) + (a)-1) & ~(a - 1))\n\nutils.h has a alignUp() function.\n\n> +\n> using namespace RPiController;\n> using namespace libcamera;\n> using libcamera::utils::Duration;\n> @@ -66,8 +70,13 @@ private:\n> \t/* Largest long exposure scale factor given as a left shift on the frame length. */\n> \tstatic constexpr int longExposureShiftMax = 7;\n> \n> +\tstatic constexpr int pdafStatsRows = 12;\n> +\tstatic constexpr int pdafStatsCols = 16;\n> +\n> \tvoid populateMetadata(const MdParser::RegisterMap ®isters,\n> \t\t\t Metadata &metadata) const override;\n> +\tstatic bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> +\t\t\t\t PdafRegions &pdaf);\n> };\n> \n> CamHelperImx519::CamHelperImx519()\n> @@ -90,6 +99,11 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> \tMdParser::RegisterMap registers;\n> \tDeviceStatus deviceStatus;\n> \n> +\tLOG(IPARPI, Debug) << \"Embedded buffer size: \" << buffer.size();\n> +\n> +\tsize_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n> +\tbytesPerLine = ALIGN_UP(bytesPerLine, 16);\n> +\n> \tif (metadata.get(\"device.status\", deviceStatus)) {\n> \t\tLOG(IPARPI, Error) << \"DeviceStatus not found from DelayedControls\";\n> \t\treturn;\n> @@ -97,6 +111,14 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> \n> \tparseEmbeddedData(buffer, metadata);\n> \n> +\tif (buffer.size() > 2 * bytesPerLine) {\n\nIf my understanding is correct, the value 2 here means that the sensor\ngenerates 2 lines of register embedded data, followed by PDAF data. A\nname constexpr would be nice to replace the magic value. I would also\npass a subspan of 2 lines to the parseEmbeddedData() function, as it\nshouldn't look into the PDAF section.\n\n> +\t\tPdafRegions pdaf;\n> +\t\tparsePdafData(&buffer[2 * bytesPerLine],\n> +\t\t\t buffer.size() - 2 * bytesPerLine,\n> +\t\t\t mode_.bitdepth, pdaf);\n> +\t\tmetadata.set(\"pdaf.data\", pdaf);\n> +\t}\n> +\n> \t/*\n> \t * The DeviceStatus struct is first populated with values obtained from\n> \t * DelayedControls. If this reports frame length is > frameLengthMax,\n> @@ -188,6 +210,34 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,\n> \tmetadata.set(\"device.status\", deviceStatus);\n> }\n> \n> +bool CamHelperImx519::parsePdafData(const uint8_t *ptr, size_t len,\n\nThis function should take a span instead of ptr + len.\n\n> +\t\t\t\t unsigned bpp, PdafRegions &pdaf)\n> +{\n> +\tsize_t step = bpp >> 1; /* bytes per PDAF grid entry */\n> +\n> +\tif (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {\n\nCan the bpp check fail, or do we always use RAW10 or RAW12 with this\nsensor ? The driver posted to the linux-media mailing list only supports\nRAW10. Does the sensor support RAW8 or RAW14 (or more) ?\n\nWhere does the 194 come from ? A named constexpr, or a mathematical\nexpression based on named constexprs, would be good too. Same for the\nfirst two bytes, magic values are not nice.\n\n> +\t\tLOG(IPARPI, Error) << \"PDAF data in unsupported format\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tpdaf.init({ pdafStatsCols, pdafStatsRows });\n> +\n> +\tptr += 2 * step;\n> +\tfor (unsigned i = 0; i < pdafStatsRows; ++i) {\n> +\t\tfor (unsigned j = 0; j < pdafStatsCols; ++j) {\n> +\t\t\tunsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n\nuint8_t\n\n> +\t\t\tint p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n\nLowercase for hex constants.\n\n> +\t\t\tPdafData pdafData;\n> +\t\t\tpdafData.conf = c;\n> +\t\t\tpdafData.phase = c ? p : 0;\n> +\t\t\tpdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> +\t\t\tptr += step;\n> +\t\t}\n> +\t}\n> +\n> +\treturn true;\n> +}\n\nNow that I've reviewed this, I realize all those commends apply to the\nIMX708 helper too. Can we factor this code out instead of duplicating it\n?\n\n> +\n> static CamHelper *create()\n> {\n> \treturn new CamHelperImx519();\n> diff --git a/src/ipa/rpi/vc4/data/imx519.json b/src/ipa/rpi/vc4/data/imx519.json\n> index 1b0a7747..0733d97e 100644\n> --- a/src/ipa/rpi/vc4/data/imx519.json\n> +++ b/src/ipa/rpi/vc4/data/imx519.json\n> @@ -350,6 +350,46 @@\n> ]\n> }\n> },\n> + {\n> + \"rpi.af\":\n> + {\n> + \"ranges\":\n> + {\n> + \"normal\":\n> + {\n> + \"min\": 0.0,\n> + \"max\": 12.0,\n> + \"default\": 1.0\n> + },\n> + \"macro\":\n> + {\n> + \"min\": 3.0,\n> + \"max\": 15.0,\n> + \"default\": 4.0\n> + }\n> + },\n> + \"speeds\":\n> + {\n> + \"normal\":\n> + {\n> + \"step_coarse\": 1.0,\n> + \"step_fine\": 0.25,\n> + \"contrast_ratio\": 0.75,\n> + \"pdaf_gain\": -0.02,\n> + \"pdaf_squelch\": 0.125,\n> + \"max_slew\": 2.0,\n> + \"pdaf_frames\": 20,\n> + \"dropout_frames\": 6,\n> + \"step_frames\": 4\n> + }\n> + },\n> + \"conf_epsilon\": 8,\n> + \"conf_thresh\": 16,\n> + \"conf_clip\": 512,\n> + \"skip_frames\": 5,\n> + \"map\": [ 0.0, 0.0, 15.0, 4095 ]\n> + }\n> + },\n> {\n> \"rpi.ccm\":\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 97245BE175\n\tfor ;\n\tFri, 23 Jun 2023 23:48:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C66C0628C0;\n\tSat, 24 Jun 2023 01:48:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 053976002B\n\tfor ;\n\tSat, 24 Jun 2023 01:48:49 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 904AF3F1;\n\tSat, 24 Jun 2023 01:48:12 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687564131;\n\tbh=g5tRngEKvvl7MGKP4YqTnkD3vIASNYqRNAUjjXm+kAs=;\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=tgewznsxSBQZUh1lW0yxElkoStx+VHqYrSgmogYB5Ove+uPPuihkYHFt83eE5le9K\n\tkhfEdCAREBlstCxFEvBxd25IWz6Uy5xHPIILtxxmzIiW2JFbz/f3uiIoK7nvmC1la8\n\ttcIeZPFnFaRt7qurul+Ye8FPP2V36063EWRNc/IK4cpIaBw8r0/ruploumYu3DusNj\n\t93dt6lpEYNiTLTOcMRweu2Bgp2PPaglEiU3UiQRJVZOevqMXWnKe3qGy4oNG43pq0E\n\tLt5ZAFN+YfQGQSs3j8D5LZwC/5G2XFogabz8Y+1onv9EQlTnjAHrLrLphjouX2J0ec\n\t2JI2TnEni5Dsw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687564092;\n\tbh=g5tRngEKvvl7MGKP4YqTnkD3vIASNYqRNAUjjXm+kAs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vdRb1Htq+JxXsD/ahki1U+8wrdPLqyO716ld96Wg5Sili4nQPOqr6FpD6N2oifPW0\n\tynriPEAnvELI/2TLqToJd896VjLEcHqpAD1TOgm0u66EX56+qIT9CyKEvO2pO3RwJ5\n\tR8t4w5DxVYeWIt5W+fPipFxe6kKEOB34rodszqB8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vdRb1Htq\"; dkim-atps=neutral","Date":"Sat, 24 Jun 2023 02:48:47 +0300","To":"Umang Jain ","Message-ID":"<20230623234847.GC1669@pendragon.ideasonboard.com>","References":"<20230622150015.45888-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230622150015.45888-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","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":"libcamera-devel@lists.libcamera.org,\n\tLee Jackson ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27438,"web_url":"https://patchwork.libcamera.org/comment/27438/","msgid":"","date":"2023-06-29T21:06:59","subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello,\n\nOn 6/24/23 1:48 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> CC'ing Naush, as a review from Raspberry Pi would be nice.\n>\n> On Thu, Jun 22, 2023 at 08:30:15PM +0530, Umang Jain via libcamera-devel wrote:\n>> From: Lee Jackson \n>>\n>> Adds the PDAF support for IMX519 camera module by Arducam.\n>>\n>> Tested-by: Umang Jain \n>> Signed-off-by: Umang Jain \n>> Signed-off-by: Lee Jackson \n>> ---\n>> Changes in v3:\n>> - Collect author's S-o-B from v2.\n>>\n>> ---\n>> src/ipa/rpi/cam_helper/cam_helper_imx519.cpp | 50 ++++++++++++++++++++\n>> src/ipa/rpi/vc4/data/imx519.json | 40 ++++++++++++++++\n>> 2 files changed, 90 insertions(+)\n>>\n>> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n>> index c7262aa0..6d032082 100644\n>> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n>> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n>> @@ -15,9 +15,13 @@\n>> \n>> #include \n>> \n>> +#include \"controller/pdaf_data.h\"\n>> +\n>> #include \"cam_helper.h\"\n>> #include \"md_parser.h\"\n>> \n>> +#define ALIGN_UP(x, a) (((x) + (a)-1) & ~(a - 1))\n> utils.h has a alignUp() function.\n>\n>> +\n>> using namespace RPiController;\n>> using namespace libcamera;\n>> using libcamera::utils::Duration;\n>> @@ -66,8 +70,13 @@ private:\n>> \t/* Largest long exposure scale factor given as a left shift on the frame length. */\n>> \tstatic constexpr int longExposureShiftMax = 7;\n>> \n>> +\tstatic constexpr int pdafStatsRows = 12;\n>> +\tstatic constexpr int pdafStatsCols = 16;\n>> +\n>> \tvoid populateMetadata(const MdParser::RegisterMap ®isters,\n>> \t\t\t Metadata &metadata) const override;\n>> +\tstatic bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n>> +\t\t\t\t PdafRegions &pdaf);\n>> };\n>> \n>> CamHelperImx519::CamHelperImx519()\n>> @@ -90,6 +99,11 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n>> \tMdParser::RegisterMap registers;\n>> \tDeviceStatus deviceStatus;\n>> \n>> +\tLOG(IPARPI, Debug) << \"Embedded buffer size: \" << buffer.size();\n>> +\n>> +\tsize_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n>> +\tbytesPerLine = ALIGN_UP(bytesPerLine, 16);\n>> +\n>> \tif (metadata.get(\"device.status\", deviceStatus)) {\n>> \t\tLOG(IPARPI, Error) << \"DeviceStatus not found from DelayedControls\";\n>> \t\treturn;\n>> @@ -97,6 +111,14 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n>> \n>> \tparseEmbeddedData(buffer, metadata);\n>> \n>> +\tif (buffer.size() > 2 * bytesPerLine) {\n> If my understanding is correct, the value 2 here means that the sensor\n> generates 2 lines of register embedded data, followed by PDAF data. A\n> name constexpr would be nice to replace the magic value. I would also\n> pass a subspan of 2 lines to the parseEmbeddedData() function, as it\n> shouldn't look into the PDAF section.\n\nI am refactoring this as mentioned in the review but having few questions?\n\nIs embedded data contains PDAF data as well ? OR embedded data is \nseparate and PDAF data is separate sections ?\n\nFor e.g. I was looking at IMX708 driver [1], and what it states is \nconfusing to me as well:\n\n/*\n  * Metadata buffer holds a variety of data, all sent with the same \nVC/DT (0x12).\n  * It comprises two scanlines (of up to 5760 bytes each, for 4608 pixels)\n  * of embedded data, one line of PDAF data, and two lines of AE-HIST data\n  * (AE histograms are valid for HDR mode and empty in non-HDR modes).\n  */\n#define IMX708_EMBEDDED_LINE_WIDTH (5 * 5760)\n#define IMX708_NUM_EMBEDDED_LINES 1\n\n\"Two scanlines of embedded data\" but macro IMX708_NUM_EMBEDDED_LINES = 1 \n? Is scanlines different ?\n\n\"One lines of PDAF data\" mentioned int the driver but libcamera-helper \n[2]  seems to consider two lines instead ?\n\nNaushir, is there any specific documentation regarding the embedded data \n/ layout etc. ? It seems I am missing context.\n\n[1]: \nhttps://github.com/raspberrypi/linux/blob/rpi-6.4.y/drivers/media/i2c/imx708.c\n[2]: \nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp#n127\n>\n>> +\t\tPdafRegions pdaf;\n>> +\t\tparsePdafData(&buffer[2 * bytesPerLine],\n>> +\t\t\t buffer.size() - 2 * bytesPerLine,\n>> +\t\t\t mode_.bitdepth, pdaf);\n>> +\t\tmetadata.set(\"pdaf.data\", pdaf);\n>> +\t}\n>> +\n>> \t/*\n>> \t * The DeviceStatus struct is first populated with values obtained from\n>> \t * DelayedControls. If this reports frame length is > frameLengthMax,\n>> @@ -188,6 +210,34 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,\n>> \tmetadata.set(\"device.status\", deviceStatus);\n>> }\n>> \n>> +bool CamHelperImx519::parsePdafData(const uint8_t *ptr, size_t len,\n> This function should take a span instead of ptr + len.\n>\n>> +\t\t\t\t unsigned bpp, PdafRegions &pdaf)\n>> +{\n>> +\tsize_t step = bpp >> 1; /* bytes per PDAF grid entry */\n>> +\n>> +\tif (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {\n> Can the bpp check fail, or do we always use RAW10 or RAW12 with this\n> sensor ? The driver posted to the linux-media mailing list only supports\n> RAW10. Does the sensor support RAW8 or RAW14 (or more) ?\n>\n> Where does the 194 come from ? A named constexpr, or a mathematical\n> expression based on named constexprs, would be good too. Same for the\n> first two bytes, magic values are not nice.\n>\n>> +\t\tLOG(IPARPI, Error) << \"PDAF data in unsupported format\";\n>> +\t\treturn false;\n>> +\t}\n>> +\n>> +\tpdaf.init({ pdafStatsCols, pdafStatsRows });\n>> +\n>> +\tptr += 2 * step;\n>> +\tfor (unsigned i = 0; i < pdafStatsRows; ++i) {\n>> +\t\tfor (unsigned j = 0; j < pdafStatsCols; ++j) {\n>> +\t\t\tunsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> uint8_t\n>\n>> +\t\t\tint p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> Lowercase for hex constants.\n>\n>> +\t\t\tPdafData pdafData;\n>> +\t\t\tpdafData.conf = c;\n>> +\t\t\tpdafData.phase = c ? p : 0;\n>> +\t\t\tpdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n>> +\t\t\tptr += step;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\treturn true;\n>> +}\n> Now that I've reviewed this, I realize all those commends apply to the\n> IMX708 helper too. Can we factor this code out instead of duplicating it\n> ?\n>\n>> +\n>> static CamHelper *create()\n>> {\n>> \treturn new CamHelperImx519();\n>> diff --git a/src/ipa/rpi/vc4/data/imx519.json b/src/ipa/rpi/vc4/data/imx519.json\n>> index 1b0a7747..0733d97e 100644\n>> --- a/src/ipa/rpi/vc4/data/imx519.json\n>> +++ b/src/ipa/rpi/vc4/data/imx519.json\n>> @@ -350,6 +350,46 @@\n>> ]\n>> }\n>> },\n>> + {\n>> + \"rpi.af\":\n>> + {\n>> + \"ranges\":\n>> + {\n>> + \"normal\":\n>> + {\n>> + \"min\": 0.0,\n>> + \"max\": 12.0,\n>> + \"default\": 1.0\n>> + },\n>> + \"macro\":\n>> + {\n>> + \"min\": 3.0,\n>> + \"max\": 15.0,\n>> + \"default\": 4.0\n>> + }\n>> + },\n>> + \"speeds\":\n>> + {\n>> + \"normal\":\n>> + {\n>> + \"step_coarse\": 1.0,\n>> + \"step_fine\": 0.25,\n>> + \"contrast_ratio\": 0.75,\n>> + \"pdaf_gain\": -0.02,\n>> + \"pdaf_squelch\": 0.125,\n>> + \"max_slew\": 2.0,\n>> + \"pdaf_frames\": 20,\n>> + \"dropout_frames\": 6,\n>> + \"step_frames\": 4\n>> + }\n>> + },\n>> + \"conf_epsilon\": 8,\n>> + \"conf_thresh\": 16,\n>> + \"conf_clip\": 512,\n>> + \"skip_frames\": 5,\n>> + \"map\": [ 0.0, 0.0, 15.0, 4095 ]\n>> + }\n>> + },\n>> {\n>> \"rpi.ccm\":\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 30DEABE175\n\tfor ;\n\tThu, 29 Jun 2023 21:07:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FE19628C0;\n\tThu, 29 Jun 2023 23:07:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D973E60386\n\tfor ;\n\tThu, 29 Jun 2023 23:07:02 +0200 (CEST)","from [192.168.0.136] (85-160-58-109.reb.o2.cz [85.160.58.109])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 784456CF;\n\tThu, 29 Jun 2023 23:06:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688072824;\n\tbh=3xLM/ghxHyHzmPZDrhueR9dJwvBqslk1o2ZLQNAsLg8=;\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=gW0K3fP0jtumAazKL0iltt80vd7ELdXXLeIqUjfB8rnn9aZJr1xqFJMY3961B/uX+\n\tw6V30OzhwOXMLRZXa13/2/12is4sicLH/kXa0h8nqDJUeFkJbK/rHNHUyEOGNP6Qqq\n\tz9YEs3zfiMQEMx4zCG9nRuqU80RJlgGHt8MQM36NTh4GBIgwdOVR1f733oXj3BW26j\n\t2biGtArTHnl3dX2W2tQG2/AcxS4mrtyh0uY9sIdmCXR55TrJk/VngKf8dDDW9v0AfO\n\txRVYMjvAkigjf6bOblb4kvkIlI4POpE/ffK8McagdfUm/VtRtlxFkace1ToOTfhdlV\n\twT7NOzCiswPUA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1688072781;\n\tbh=3xLM/ghxHyHzmPZDrhueR9dJwvBqslk1o2ZLQNAsLg8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Uib4Iq6GYaZoKLpPtddjsSvNQdbETMPXbkb32x9oOuoSA7/wz6DCmyGMYpN9Mq3eW\n\tiOc1wF2mBlmAVXfgoLRXyY1/8iy53zI0CR+qtfU2m2UeLVGr8eiz3g7t+OmEbr6NdO\n\tEs5KNdIz0btw58a8R7WIO1qGIg8VDgGvD5RXurvo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Uib4Iq6G\"; dkim-atps=neutral","Message-ID":"","Date":"Thu, 29 Jun 2023 23:06:59 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","To":"Laurent Pinchart ","References":"<20230622150015.45888-1-umang.jain@ideasonboard.com>\n\t<20230623234847.GC1669@pendragon.ideasonboard.com>","Content-Language":"en-US","In-Reply-To":"<20230623234847.GC1669@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27439,"web_url":"https://patchwork.libcamera.org/comment/27439/","msgid":"","date":"2023-06-30T07:52:49","subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Umang,\n\nOn Thu, 29 Jun 2023 at 22:07, Umang Jain wrote:\n>\n> Hello,\n>\n> On 6/24/23 1:48 AM, Laurent Pinchart wrote:\n> > Hi Umang,\n> >\n> > Thank you for the patch.\n> >\n> > CC'ing Naush, as a review from Raspberry Pi would be nice.\n> >\n> > On Thu, Jun 22, 2023 at 08:30:15PM +0530, Umang Jain via libcamera-devel wrote:\n> >> From: Lee Jackson \n> >>\n> >> Adds the PDAF support for IMX519 camera module by Arducam.\n> >>\n> >> Tested-by: Umang Jain \n> >> Signed-off-by: Umang Jain \n> >> Signed-off-by: Lee Jackson \n> >> ---\n> >> Changes in v3:\n> >> - Collect author's S-o-B from v2.\n> >>\n> >> ---\n> >> src/ipa/rpi/cam_helper/cam_helper_imx519.cpp | 50 ++++++++++++++++++++\n> >> src/ipa/rpi/vc4/data/imx519.json | 40 ++++++++++++++++\n> >> 2 files changed, 90 insertions(+)\n> >>\n> >> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> >> index c7262aa0..6d032082 100644\n> >> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> >> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> >> @@ -15,9 +15,13 @@\n> >>\n> >> #include \n> >>\n> >> +#include \"controller/pdaf_data.h\"\n> >> +\n> >> #include \"cam_helper.h\"\n> >> #include \"md_parser.h\"\n> >>\n> >> +#define ALIGN_UP(x, a) (((x) + (a)-1) & ~(a - 1))\n> > utils.h has a alignUp() function.\n> >\n> >> +\n> >> using namespace RPiController;\n> >> using namespace libcamera;\n> >> using libcamera::utils::Duration;\n> >> @@ -66,8 +70,13 @@ private:\n> >> /* Largest long exposure scale factor given as a left shift on the frame length. */\n> >> static constexpr int longExposureShiftMax = 7;\n> >>\n> >> + static constexpr int pdafStatsRows = 12;\n> >> + static constexpr int pdafStatsCols = 16;\n> >> +\n> >> void populateMetadata(const MdParser::RegisterMap ®isters,\n> >> Metadata &metadata) const override;\n> >> + static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> >> + PdafRegions &pdaf);\n> >> };\n> >>\n> >> CamHelperImx519::CamHelperImx519()\n> >> @@ -90,6 +99,11 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> >> MdParser::RegisterMap registers;\n> >> DeviceStatus deviceStatus;\n> >>\n> >> + LOG(IPARPI, Debug) << \"Embedded buffer size: \" << buffer.size();\n> >> +\n> >> + size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n> >> + bytesPerLine = ALIGN_UP(bytesPerLine, 16);\n> >> +\n> >> if (metadata.get(\"device.status\", deviceStatus)) {\n> >> LOG(IPARPI, Error) << \"DeviceStatus not found from DelayedControls\";\n> >> return;\n> >> @@ -97,6 +111,14 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> >>\n> >> parseEmbeddedData(buffer, metadata);\n> >>\n> >> + if (buffer.size() > 2 * bytesPerLine) {\n> > If my understanding is correct, the value 2 here means that the sensor\n> > generates 2 lines of register embedded data, followed by PDAF data. A\n> > name constexpr would be nice to replace the magic value. I would also\n> > pass a subspan of 2 lines to the parseEmbeddedData() function, as it\n> > shouldn't look into the PDAF section.\n>\n> I am refactoring this as mentioned in the review but having few questions?\n>\n> Is embedded data contains PDAF data as well ? OR embedded data is\n> separate and PDAF data is separate sections ?\n\nPDAF data comes after embedded data - typically with a different packet id.\nHowever, because Unicam only has 2x DMA channels, embedded data, PDAF data and\nHDR histogram data all get routed into a single metadata buffer.\n\n>\n> For e.g. I was looking at IMX708 driver [1], and what it states is\n> confusing to me as well:\n>\n> /*\n> * Metadata buffer holds a variety of data, all sent with the same\n> VC/DT (0x12).\n> * It comprises two scanlines (of up to 5760 bytes each, for 4608 pixels)\n> * of embedded data, one line of PDAF data, and two lines of AE-HIST data\n> * (AE histograms are valid for HDR mode and empty in non-HDR modes).\n> */\n> #define IMX708_EMBEDDED_LINE_WIDTH (5 * 5760)\n> #define IMX708_NUM_EMBEDDED_LINES 1\n>\n> \"Two scanlines of embedded data\" but macro IMX708_NUM_EMBEDDED_LINES = 1\n> ? Is scanlines different ?\n\nYou should probably ignore this value. As you know we don't currently have\nofficial V4L2 support for embedded data or generic sensor metadata. For our\ndownstream workaround this define is only used to size up the metadata buffer\n(since formats have to have a width and height in V4L2). It conveys no\ninformation on the actual embedded data structure.\n\n>\n> \"One lines of PDAF data\" mentioned int the driver but libcamera-helper\n> [2] seems to consider two lines instead ?\n>\n\nThe actual number of embedded data scan lines is 2.\n\n> Naushir, is there any specific documentation regarding the embedded data\n> / layout etc. ? It seems I am missing context.\n\nThe datasheet for the imx708 is under strict NDA with Sony so unfortunately I\ncannot share any more than what is already in the parsing code here.\n\nI should also qualify that this is specific to the IMX708. I don't know\nanything about structure of the IMX519 metadata, but given the parsing code\nseems to give some sensible results, it may very well be the same.\n\nRegards,\nNaush\n\n>\n> [1]:\n> https://github.com/raspberrypi/linux/blob/rpi-6.4.y/drivers/media/i2c/imx708.c\n> [2]:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp#n127\n> >\n> >> + PdafRegions pdaf;\n> >> + parsePdafData(&buffer[2 * bytesPerLine],\n> >> + buffer.size() - 2 * bytesPerLine,\n> >> + mode_.bitdepth, pdaf);\n> >> + metadata.set(\"pdaf.data\", pdaf);\n> >> + }\n> >> +\n> >> /*\n> >> * The DeviceStatus struct is first populated with values obtained from\n> >> * DelayedControls. If this reports frame length is > frameLengthMax,\n> >> @@ -188,6 +210,34 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,\n> >> metadata.set(\"device.status\", deviceStatus);\n> >> }\n> >>\n> >> +bool CamHelperImx519::parsePdafData(const uint8_t *ptr, size_t len,\n> > This function should take a span instead of ptr + len.\n> >\n> >> + unsigned bpp, PdafRegions &pdaf)\n> >> +{\n> >> + size_t step = bpp >> 1; /* bytes per PDAF grid entry */\n> >> +\n> >> + if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {\n> > Can the bpp check fail, or do we always use RAW10 or RAW12 with this\n> > sensor ? The driver posted to the linux-media mailing list only supports\n> > RAW10. Does the sensor support RAW8 or RAW14 (or more) ?\n> >\n> > Where does the 194 come from ? A named constexpr, or a mathematical\n> > expression based on named constexprs, would be good too. Same for the\n> > first two bytes, magic values are not nice.\n> >\n> >> + LOG(IPARPI, Error) << \"PDAF data in unsupported format\";\n> >> + return false;\n> >> + }\n> >> +\n> >> + pdaf.init({ pdafStatsCols, pdafStatsRows });\n> >> +\n> >> + ptr += 2 * step;\n> >> + for (unsigned i = 0; i < pdafStatsRows; ++i) {\n> >> + for (unsigned j = 0; j < pdafStatsCols; ++j) {\n> >> + unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> > uint8_t\n> >\n> >> + int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> > Lowercase for hex constants.\n> >\n> >> + PdafData pdafData;\n> >> + pdafData.conf = c;\n> >> + pdafData.phase = c ? p : 0;\n> >> + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> >> + ptr += step;\n> >> + }\n> >> + }\n> >> +\n> >> + return true;\n> >> +}\n> > Now that I've reviewed this, I realize all those commends apply to the\n> > IMX708 helper too. Can we factor this code out instead of duplicating it\n> > ?\n> >\n> >> +\n> >> static CamHelper *create()\n> >> {\n> >> return new CamHelperImx519();\n> >> diff --git a/src/ipa/rpi/vc4/data/imx519.json b/src/ipa/rpi/vc4/data/imx519.json\n> >> index 1b0a7747..0733d97e 100644\n> >> --- a/src/ipa/rpi/vc4/data/imx519.json\n> >> +++ b/src/ipa/rpi/vc4/data/imx519.json\n> >> @@ -350,6 +350,46 @@\n> >> ]\n> >> }\n> >> },\n> >> + {\n> >> + \"rpi.af\":\n> >> + {\n> >> + \"ranges\":\n> >> + {\n> >> + \"normal\":\n> >> + {\n> >> + \"min\": 0.0,\n> >> + \"max\": 12.0,\n> >> + \"default\": 1.0\n> >> + },\n> >> + \"macro\":\n> >> + {\n> >> + \"min\": 3.0,\n> >> + \"max\": 15.0,\n> >> + \"default\": 4.0\n> >> + }\n> >> + },\n> >> + \"speeds\":\n> >> + {\n> >> + \"normal\":\n> >> + {\n> >> + \"step_coarse\": 1.0,\n> >> + \"step_fine\": 0.25,\n> >> + \"contrast_ratio\": 0.75,\n> >> + \"pdaf_gain\": -0.02,\n> >> + \"pdaf_squelch\": 0.125,\n> >> + \"max_slew\": 2.0,\n> >> + \"pdaf_frames\": 20,\n> >> + \"dropout_frames\": 6,\n> >> + \"step_frames\": 4\n> >> + }\n> >> + },\n> >> + \"conf_epsilon\": 8,\n> >> + \"conf_thresh\": 16,\n> >> + \"conf_clip\": 512,\n> >> + \"skip_frames\": 5,\n> >> + \"map\": [ 0.0, 0.0, 15.0, 4095 ]\n> >> + }\n> >> + },\n> >> {\n> >> \"rpi.ccm\":\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 F22F3BDB1D\n\tfor ;\n\tFri, 30 Jun 2023 07:53:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19FC4628C0;\n\tFri, 30 Jun 2023 09:53:09 +0200 (CEST)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E81CA60384\n\tfor ;\n\tFri, 30 Jun 2023 09:53:06 +0200 (CEST)","by mail-yb1-xb36.google.com with SMTP id\n\t3f1490d57ef6-bacf685150cso1457714276.3\n\tfor ;\n\tFri, 30 Jun 2023 00:53:06 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688111589;\n\tbh=RCHAhH/h3Bq7JVm9sIgqMXx7oNVsTSfIKOH1Yd0Hh0Q=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=UGOgI3kEqmR1xSOUEcjXznQupPVBinIUgNPK3dVu960HyyReDF2pGwdZ+voG39Dzc\n\tTkD2Jy7pey1YS8OY8Bue1myQKMf4yDm4zfg0EiCcDRtBiEjzWqP+gsaFhW+i5Ka5x/\n\tDosyybSQizk3rA0Q6rjmUQPhKwvZ7Po9BGimt+Zqf59QJM+EBiuF4CL+gTB99RuJet\n\tyVZ3NJ6YeHMCVjN1rodjLufXfHiggGiEnbc7NxDTWDAt36M2j+/qCj8vdMYcisf42g\n\tb6I/KbKL+D7cJZ1AX1/xe0ap4Da+4sQKS0QgE7C6wbxB0OQdklQzS7GpZJs2QkiiMX\n\tFM0QtmqMOsVFQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1688111585; x=1690703585;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=x/v7Pc+4aKHYr6fQFWJhNkP0pk9u3IM8it7Om0lZtwI=;\n\tb=g4ArdE/c9/WLyiTK6nyK6rlhZoVUcYrRy1hl7xkgvAhDRoVBaneRjq6jW2qV7jjTey\n\tDGph63lxEe9iBRfZ6PmhXSZ0fDhFxt6csVql2PES9QbAtv2ApWQ/xkyv5T+frdL7rQM0\n\t1BK/BbdApb5ldzE2B1Hqs3TGhJHzgCH/dllsMcIjO5/P2vRJluMANRuOlq63jQvWj/Ym\n\tnFXZ2iQNk6A6y7btoLfo/wJVsMmDhyv1qn764w6Znby6u8RwjDTIzjsFFZfdSvRwXeHW\n\t+Ex5x9/Jrr0KhUcmuBt0Z7n5g6AdE528GwLE/tJsRsMPCRUzA1ZDzadVqkwgny9RpCIE\n\tQiIg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"g4ArdE/c\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1688111585; x=1690703585;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=x/v7Pc+4aKHYr6fQFWJhNkP0pk9u3IM8it7Om0lZtwI=;\n\tb=YtJCSTNS8xhaXoTf3L864tncmaOkSM73VbqZ8JXru1olDrztTFguHd3ahdzhJPdBrk\n\txGZ5lYRB50R4BeTuw0ofeMYzZaCQX8fs4dhs5HUp7Z+aDddqYDpg7O/QjH2zufXPMDuY\n\tNZ9C2mcKSjN/6F4uf5C+qrMKNd596SSCiFZms+uczDw+DnQ3Df9K1RbmTjQZMHIDllCb\n\tPwmJxd2jz5yfOrwdH9Jcpw4zee9wQ8UHKk+8z0TFH4R/aV7scXUV2+5fI/olTyfwg0Ri\n\t0162uOFjwonEdlYpK6wDwVqH9CSlK1K0Yz5MYsTsn7AO3soRSHo2lQh0F+KWoTQtAoKN\n\tBLEw==","X-Gm-Message-State":"ABy/qLbxi9/fJN+iGu/oNYxKSjd7Bn4G4mJXFjEmium3N4Z0ZfMP/N7N\n\tF6iNnyA302LlNM2h9UfAaw8obMeFgipak0viSakLEA==","X-Google-Smtp-Source":"APBJJlEWvccTirVGKVzQ/wyQdMAL6MljTGKdWUmfguL0vmmXiHdUJtSBjbhZ7FpDG+qyKju/XFVDBZHfxEzUHv/RyaA=","X-Received":"by 2002:a81:a149:0:b0:577:2570:edcc with SMTP id\n\ty70-20020a81a149000000b005772570edccmr1432653ywg.52.1688111585192;\n\tFri, 30 Jun 2023 00:53:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20230622150015.45888-1-umang.jain@ideasonboard.com>\n\t<20230623234847.GC1669@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","Date":"Fri, 30 Jun 2023 08:52:49 +0100","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27440,"web_url":"https://patchwork.libcamera.org/comment/27440/","msgid":"","date":"2023-06-30T10:55:54","subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Umang\n\nOn Fri, 30 Jun 2023 at 08:53, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Hi Umang,\n>\n> On Thu, 29 Jun 2023 at 22:07, Umang Jain wrote:\n> >\n> > Hello,\n> >\n> > On 6/24/23 1:48 AM, Laurent Pinchart wrote:\n> > > Hi Umang,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > CC'ing Naush, as a review from Raspberry Pi would be nice.\n> > >\n> > > On Thu, Jun 22, 2023 at 08:30:15PM +0530, Umang Jain via libcamera-devel wrote:\n> > >> From: Lee Jackson \n> > >>\n> > >> Adds the PDAF support for IMX519 camera module by Arducam.\n> > >>\n> > >> Tested-by: Umang Jain \n> > >> Signed-off-by: Umang Jain \n> > >> Signed-off-by: Lee Jackson \n> > >> ---\n> > >> Changes in v3:\n> > >> - Collect author's S-o-B from v2.\n> > >>\n> > >> ---\n> > >> src/ipa/rpi/cam_helper/cam_helper_imx519.cpp | 50 ++++++++++++++++++++\n> > >> src/ipa/rpi/vc4/data/imx519.json | 40 ++++++++++++++++\n> > >> 2 files changed, 90 insertions(+)\n> > >>\n> > >> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> > >> index c7262aa0..6d032082 100644\n> > >> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> > >> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> > >> @@ -15,9 +15,13 @@\n> > >>\n> > >> #include \n> > >>\n> > >> +#include \"controller/pdaf_data.h\"\n> > >> +\n> > >> #include \"cam_helper.h\"\n> > >> #include \"md_parser.h\"\n> > >>\n> > >> +#define ALIGN_UP(x, a) (((x) + (a)-1) & ~(a - 1))\n> > > utils.h has a alignUp() function.\n> > >\n> > >> +\n> > >> using namespace RPiController;\n> > >> using namespace libcamera;\n> > >> using libcamera::utils::Duration;\n> > >> @@ -66,8 +70,13 @@ private:\n> > >> /* Largest long exposure scale factor given as a left shift on the frame length. */\n> > >> static constexpr int longExposureShiftMax = 7;\n> > >>\n> > >> + static constexpr int pdafStatsRows = 12;\n> > >> + static constexpr int pdafStatsCols = 16;\n> > >> +\n> > >> void populateMetadata(const MdParser::RegisterMap ®isters,\n> > >> Metadata &metadata) const override;\n> > >> + static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> > >> + PdafRegions &pdaf);\n> > >> };\n> > >>\n> > >> CamHelperImx519::CamHelperImx519()\n> > >> @@ -90,6 +99,11 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> > >> MdParser::RegisterMap registers;\n> > >> DeviceStatus deviceStatus;\n> > >>\n> > >> + LOG(IPARPI, Debug) << \"Embedded buffer size: \" << buffer.size();\n> > >> +\n> > >> + size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n> > >> + bytesPerLine = ALIGN_UP(bytesPerLine, 16);\n> > >> +\n> > >> if (metadata.get(\"device.status\", deviceStatus)) {\n> > >> LOG(IPARPI, Error) << \"DeviceStatus not found from DelayedControls\";\n> > >> return;\n> > >> @@ -97,6 +111,14 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> > >>\n> > >> parseEmbeddedData(buffer, metadata);\n> > >>\n> > >> + if (buffer.size() > 2 * bytesPerLine) {\n> > > If my understanding is correct, the value 2 here means that the sensor\n> > > generates 2 lines of register embedded data, followed by PDAF data. A\n> > > name constexpr would be nice to replace the magic value. I would also\n> > > pass a subspan of 2 lines to the parseEmbeddedData() function, as it\n> > > shouldn't look into the PDAF section.\n> >\n> > I am refactoring this as mentioned in the review but having few questions?\n> >\n> > Is embedded data contains PDAF data as well ? OR embedded data is\n> > separate and PDAF data is separate sections ?\n>\n> PDAF data comes after embedded data - typically with a different packet id.\n> However, because Unicam only has 2x DMA channels, embedded data, PDAF data and\n> HDR histogram data all get routed into a single metadata buffer.\n\nFor IMX708 the CSI2 data type for additional metadata lines (PDAF, AE,\nhistograms, etc) is configurable, but our downstream driver sets them\nto use 0x12 to be the same as the embedded data. As Naush says, it\nmakes no difference with Unicam as that only has 2 data paths (image,\nand \"everything else\"), but keeps it a little cleaner.\n\n> >\n> > For e.g. I was looking at IMX708 driver [1], and what it states is\n> > confusing to me as well:\n> >\n> > /*\n> > * Metadata buffer holds a variety of data, all sent with the same\n> > VC/DT (0x12).\n> > * It comprises two scanlines (of up to 5760 bytes each, for 4608 pixels)\n> > * of embedded data, one line of PDAF data, and two lines of AE-HIST data\n> > * (AE histograms are valid for HDR mode and empty in non-HDR modes).\n> > */\n> > #define IMX708_EMBEDDED_LINE_WIDTH (5 * 5760)\n> > #define IMX708_NUM_EMBEDDED_LINES 1\n> >\n> > \"Two scanlines of embedded data\" but macro IMX708_NUM_EMBEDDED_LINES = 1\n> > ? Is scanlines different ?\n>\n> You should probably ignore this value. As you know we don't currently have\n> official V4L2 support for embedded data or generic sensor metadata. For our\n> downstream workaround this define is only used to size up the metadata buffer\n> (since formats have to have a width and height in V4L2). It conveys no\n> information on the actual embedded data structure.\n\nThe \"everything else\" channel in Unicam has no stride / bytesperline\nconfiguration, therefore it will write the data exactly as it comes\nin. For most sensors the \"line length\" of the metadata will be the\nsame as that for the image mode, therefore the parser needs to know\nthe image format too.\nThe order that the sensor produces the lines of metadata is stated in\nthe datasheet, so the parser can know what to expect and in what\norder.\n\nIf a CSI2 receiver supports demultiplexing more data streams, and the\ndriver is amended to use different data id values, then the PDAF data\ncould be in a separate buffer to embedded data etc. So if you're\nreally trying to make it generic then you want to be passing in a\npointer to the start of the metadata.\n\nI would expect this to all change when Sakari lands the \"Generic line\nbased metadata support\" series that was discussed in Prague earlier\nthis week.\n\n> >\n> > \"One lines of PDAF data\" mentioned int the driver but libcamera-helper\n> > [2] seems to consider two lines instead ?\n> >\n>\n> The actual number of embedded data scan lines is 2.\n>\n> > Naushir, is there any specific documentation regarding the embedded data\n> > / layout etc. ? It seems I am missing context.\n>\n> The datasheet for the imx708 is under strict NDA with Sony so unfortunately I\n> cannot share any more than what is already in the parsing code here.\n>\n> I should also qualify that this is specific to the IMX708. I don't know\n> anything about structure of the IMX519 metadata, but given the parsing code\n> seems to give some sensible results, it may very well be the same.\n\nNote that Arducam have just made a PR adding PDAF to the kernel driver\nfor their 64MPix camera (IMX682). They don't appear to have added PDAF\nsupport to their libcamera fork[2], but I would expect it to happen\nrelatively soon. Perhaps it's worth waiting for that just to ensure\nyou're covering it too.\n\n Dave\n\n[1] https://github.com/raspberrypi/linux/pull/5523\n[2] https://github.com/arducam/libcamera\n\n> Regards,\n> Naush\n>\n> >\n> > [1]:\n> > https://github.com/raspberrypi/linux/blob/rpi-6.4.y/drivers/media/i2c/imx708.c\n> > [2]:\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp#n127\n> > >\n> > >> + PdafRegions pdaf;\n> > >> + parsePdafData(&buffer[2 * bytesPerLine],\n> > >> + buffer.size() - 2 * bytesPerLine,\n> > >> + mode_.bitdepth, pdaf);\n> > >> + metadata.set(\"pdaf.data\", pdaf);\n> > >> + }\n> > >> +\n> > >> /*\n> > >> * The DeviceStatus struct is first populated with values obtained from\n> > >> * DelayedControls. If this reports frame length is > frameLengthMax,\n> > >> @@ -188,6 +210,34 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,\n> > >> metadata.set(\"device.status\", deviceStatus);\n> > >> }\n> > >>\n> > >> +bool CamHelperImx519::parsePdafData(const uint8_t *ptr, size_t len,\n> > > This function should take a span instead of ptr + len.\n> > >\n> > >> + unsigned bpp, PdafRegions &pdaf)\n> > >> +{\n> > >> + size_t step = bpp >> 1; /* bytes per PDAF grid entry */\n> > >> +\n> > >> + if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {\n> > > Can the bpp check fail, or do we always use RAW10 or RAW12 with this\n> > > sensor ? The driver posted to the linux-media mailing list only supports\n> > > RAW10. Does the sensor support RAW8 or RAW14 (or more) ?\n> > >\n> > > Where does the 194 come from ? A named constexpr, or a mathematical\n> > > expression based on named constexprs, would be good too. Same for the\n> > > first two bytes, magic values are not nice.\n> > >\n> > >> + LOG(IPARPI, Error) << \"PDAF data in unsupported format\";\n> > >> + return false;\n> > >> + }\n> > >> +\n> > >> + pdaf.init({ pdafStatsCols, pdafStatsRows });\n> > >> +\n> > >> + ptr += 2 * step;\n> > >> + for (unsigned i = 0; i < pdafStatsRows; ++i) {\n> > >> + for (unsigned j = 0; j < pdafStatsCols; ++j) {\n> > >> + unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> > > uint8_t\n> > >\n> > >> + int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> > > Lowercase for hex constants.\n> > >\n> > >> + PdafData pdafData;\n> > >> + pdafData.conf = c;\n> > >> + pdafData.phase = c ? p : 0;\n> > >> + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> > >> + ptr += step;\n> > >> + }\n> > >> + }\n> > >> +\n> > >> + return true;\n> > >> +}\n> > > Now that I've reviewed this, I realize all those commends apply to the\n> > > IMX708 helper too. Can we factor this code out instead of duplicating it\n> > > ?\n> > >\n> > >> +\n> > >> static CamHelper *create()\n> > >> {\n> > >> return new CamHelperImx519();\n> > >> diff --git a/src/ipa/rpi/vc4/data/imx519.json b/src/ipa/rpi/vc4/data/imx519.json\n> > >> index 1b0a7747..0733d97e 100644\n> > >> --- a/src/ipa/rpi/vc4/data/imx519.json\n> > >> +++ b/src/ipa/rpi/vc4/data/imx519.json\n> > >> @@ -350,6 +350,46 @@\n> > >> ]\n> > >> }\n> > >> },\n> > >> + {\n> > >> + \"rpi.af\":\n> > >> + {\n> > >> + \"ranges\":\n> > >> + {\n> > >> + \"normal\":\n> > >> + {\n> > >> + \"min\": 0.0,\n> > >> + \"max\": 12.0,\n> > >> + \"default\": 1.0\n> > >> + },\n> > >> + \"macro\":\n> > >> + {\n> > >> + \"min\": 3.0,\n> > >> + \"max\": 15.0,\n> > >> + \"default\": 4.0\n> > >> + }\n> > >> + },\n> > >> + \"speeds\":\n> > >> + {\n> > >> + \"normal\":\n> > >> + {\n> > >> + \"step_coarse\": 1.0,\n> > >> + \"step_fine\": 0.25,\n> > >> + \"contrast_ratio\": 0.75,\n> > >> + \"pdaf_gain\": -0.02,\n> > >> + \"pdaf_squelch\": 0.125,\n> > >> + \"max_slew\": 2.0,\n> > >> + \"pdaf_frames\": 20,\n> > >> + \"dropout_frames\": 6,\n> > >> + \"step_frames\": 4\n> > >> + }\n> > >> + },\n> > >> + \"conf_epsilon\": 8,\n> > >> + \"conf_thresh\": 16,\n> > >> + \"conf_clip\": 512,\n> > >> + \"skip_frames\": 5,\n> > >> + \"map\": [ 0.0, 0.0, 15.0, 4095 ]\n> > >> + }\n> > >> + },\n> > >> {\n> > >> \"rpi.ccm\":\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 C799FBE175\n\tfor ;\n\tFri, 30 Jun 2023 10:56:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4D4A628C0;\n\tFri, 30 Jun 2023 12:56:13 +0200 (CEST)","from mail-ua1-x92a.google.com (mail-ua1-x92a.google.com\n\t[IPv6:2607:f8b0:4864:20::92a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89F6460454\n\tfor ;\n\tFri, 30 Jun 2023 12:56:12 +0200 (CEST)","by mail-ua1-x92a.google.com with SMTP id\n\ta1e0cc1a2514c-793f262b6a9so622249241.1\n\tfor ;\n\tFri, 30 Jun 2023 03:56:12 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688122573;\n\tbh=C+vn3IhKVvcp7Xu6eOKg4jj4HzUEYIUD2LQkJ3Yj/IY=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iEBLpbPLXf1l8C54WlsVPaaRa7v7MFRCggck04Jk4v1KHtWw7BoxFEfPGuyN5AU9e\n\tFGtQnuhDVOCUXyJO7tYMwOzuDxwYse7nl0fCAHGWln50jl365ZkiDKVMdNeCIVj/lN\n\tmnIII/A8mvyHM1FHmACKTJ3HOBmhHaXMubHhe9GL6z6T8m0tDa8fFTC4aMkjiZs6p1\n\t10qDFjyKXeHC0jNgcGBXga/7P5gb8limKrCYVauu0M9Mz+C/Tpk0kls0FKEBjiOKOv\n\txh+VRSOaLzeY+zuqy9j7zpEWIcCOP+xQzBVRIjTObcDxuz7bfZzX0TvcQfOhAY8wd3\n\tAz1V6ZYYBEC0Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1688122571; x=1690714571;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=L2EPf6j4kogMfu6kAm0L8A3g1kLAhz2IpOP8sZmjZgY=;\n\tb=CLPJDrZDqe9trUPBkPC9h9jTDXMDD7udoVJtjjXNGLLHl30frYKoevzTBQOTWvqnDs\n\t2SWJZBwsWjYZ0Lw+24P6SU6VIlF/VlY+R6PJPb6a/rIU/DKukqefYSkNhULEDPauAmIg\n\tEud9AJk4sAthDNZO/NudtocIjFGuBCKndDuUzGKmZUq+gNboJ+dasZUBKH4zbFfaWhte\n\tMJ5fqdzamFTsp/bUXpNCMa62wk2c0Wb0+yJDoxWgH+R8RvnyTqA3Li1Kwwv6w2IF+CAL\n\tG06AGdf1rLKdkl+2NrbzzuEbL4dKMNVvhCvboFF1riGQOeA/E7aNsjxpVqNhp3B/9DAZ\n\tCJqA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"CLPJDrZD\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1688122571; x=1690714571;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=L2EPf6j4kogMfu6kAm0L8A3g1kLAhz2IpOP8sZmjZgY=;\n\tb=S6++ExJ/a6aEzvEjiO6/zx+IjBHjIM+h46vWAM/egvQFPJUHx7PE/2Ay8pxo9Z9BuY\n\t/fLP/RtGOrYHArUdnKkNAtjtzmAE1xQ4X4k/4Y8eYlbyumG4A8uTijTNVxsSOYjDQlqp\n\teyyF6XV5pvC29GIlO7c6ilXTUItSAGksOztGRnGUjf5yvN3sYm9PVkHwjh2Dy8912CuE\n\tePmFzhI3JJW+3OpUuxEcGniQXgpPXAjeLX2NMZHPxd7rjw3E9C71CjWfmrXI+LeVGSWF\n\tR/JyNnvDwUW/wDcPxIOKNBisMImWd5spKRkE3BJLX0CuxjEDddXNWl8uz9lP0ZSLdLRR\n\tKADA==","X-Gm-Message-State":"ABy/qLZJCmsBdIVERPJCbNUb6m4gXNaI1H7aQ77IYO6sQrW9StTux//9\n\t+R93IFQmuNz0nvA4lxPm3BQmTRSfYhV28RZxJQD8OQ==","X-Google-Smtp-Source":"APBJJlG84PU8Wfq1KVQ8+fQtN+dFLtFow/JbW5fau4QNjPqquBqh7VXKokig8muhj2AXP/UrVNZM18j17AKvqVw8wFs=","X-Received":"by 2002:a67:f655:0:b0:443:68c0:44ad with SMTP id\n\tu21-20020a67f655000000b0044368c044admr1374885vso.25.1688122571159;\n\tFri, 30 Jun 2023 03:56:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20230622150015.45888-1-umang.jain@ideasonboard.com>\n\t<20230623234847.GC1669@pendragon.ideasonboard.com>\n\t\n\t","In-Reply-To":"","Date":"Fri, 30 Jun 2023 11:55:54 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","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":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27441,"web_url":"https://patchwork.libcamera.org/comment/27441/","msgid":"<4c70957e-78be-1957-f8b2-2ba707d4135a@ideasonboard.com>","date":"2023-06-30T11:39:37","subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Dave, Naushir,\n\nOn 6/30/23 12:55 PM, Dave Stevenson wrote:\n> Hi Umang\n>\n> On Fri, 30 Jun 2023 at 08:53, Naushir Patuck via libcamera-devel\n> wrote:\n>> Hi Umang,\n>>\n>> On Thu, 29 Jun 2023 at 22:07, Umang Jain wrote:\n>>> Hello,\n>>>\n>>> On 6/24/23 1:48 AM, Laurent Pinchart wrote:\n>>>> Hi Umang,\n>>>>\n>>>> Thank you for the patch.\n>>>>\n>>>> CC'ing Naush, as a review from Raspberry Pi would be nice.\n>>>>\n>>>> On Thu, Jun 22, 2023 at 08:30:15PM +0530, Umang Jain via libcamera-devel wrote:\n>>>>> From: Lee Jackson \n>>>>>\n>>>>> Adds the PDAF support for IMX519 camera module by Arducam.\n>>>>>\n>>>>> Tested-by: Umang Jain \n>>>>> Signed-off-by: Umang Jain \n>>>>> Signed-off-by: Lee Jackson \n>>>>> ---\n>>>>> Changes in v3:\n>>>>> - Collect author's S-o-B from v2.\n>>>>>\n>>>>> ---\n>>>>> src/ipa/rpi/cam_helper/cam_helper_imx519.cpp | 50 ++++++++++++++++++++\n>>>>> src/ipa/rpi/vc4/data/imx519.json | 40 ++++++++++++++++\n>>>>> 2 files changed, 90 insertions(+)\n>>>>>\n>>>>> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n>>>>> index c7262aa0..6d032082 100644\n>>>>> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n>>>>> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n>>>>> @@ -15,9 +15,13 @@\n>>>>>\n>>>>> #include \n>>>>>\n>>>>> +#include \"controller/pdaf_data.h\"\n>>>>> +\n>>>>> #include \"cam_helper.h\"\n>>>>> #include \"md_parser.h\"\n>>>>>\n>>>>> +#define ALIGN_UP(x, a) (((x) + (a)-1) & ~(a - 1))\n>>>> utils.h has a alignUp() function.\n>>>>\n>>>>> +\n>>>>> using namespace RPiController;\n>>>>> using namespace libcamera;\n>>>>> using libcamera::utils::Duration;\n>>>>> @@ -66,8 +70,13 @@ private:\n>>>>> /* Largest long exposure scale factor given as a left shift on the frame length. */\n>>>>> static constexpr int longExposureShiftMax = 7;\n>>>>>\n>>>>> + static constexpr int pdafStatsRows = 12;\n>>>>> + static constexpr int pdafStatsCols = 16;\n>>>>> +\n>>>>> void populateMetadata(const MdParser::RegisterMap ®isters,\n>>>>> Metadata &metadata) const override;\n>>>>> + static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n>>>>> + PdafRegions &pdaf);\n>>>>> };\n>>>>>\n>>>>> CamHelperImx519::CamHelperImx519()\n>>>>> @@ -90,6 +99,11 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n>>>>> MdParser::RegisterMap registers;\n>>>>> DeviceStatus deviceStatus;\n>>>>>\n>>>>> + LOG(IPARPI, Debug) << \"Embedded buffer size: \" << buffer.size();\n>>>>> +\n>>>>> + size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n>>>>> + bytesPerLine = ALIGN_UP(bytesPerLine, 16);\n>>>>> +\n>>>>> if (metadata.get(\"device.status\", deviceStatus)) {\n>>>>> LOG(IPARPI, Error) << \"DeviceStatus not found from DelayedControls\";\n>>>>> return;\n>>>>> @@ -97,6 +111,14 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n>>>>>\n>>>>> parseEmbeddedData(buffer, metadata);\n>>>>>\n>>>>> + if (buffer.size() > 2 * bytesPerLine) {\n>>>> If my understanding is correct, the value 2 here means that the sensor\n>>>> generates 2 lines of register embedded data, followed by PDAF data. A\n>>>> name constexpr would be nice to replace the magic value. I would also\n>>>> pass a subspan of 2 lines to the parseEmbeddedData() function, as it\n>>>> shouldn't look into the PDAF section.\n>>> I am refactoring this as mentioned in the review but having few questions?\n>>>\n>>> Is embedded data contains PDAF data as well ? OR embedded data is\n>>> separate and PDAF data is separate sections ?\n>> PDAF data comes after embedded data - typically with a different packet id.\n>> However, because Unicam only has 2x DMA channels, embedded data, PDAF data and\n>> HDR histogram data all get routed into a single metadata buffer.\n> For IMX708 the CSI2 data type for additional metadata lines (PDAF, AE,\n> histograms, etc) is configurable, but our downstream driver sets them\n> to use 0x12 to be the same as the embedded data. As Naush says, it\n> makes no difference with Unicam as that only has 2 data paths (image,\n> and \"everything else\"), but keeps it a little cleaner.\n>\n>>> For e.g. I was looking at IMX708 driver [1], and what it states is\n>>> confusing to me as well:\n>>>\n>>> /*\n>>> * Metadata buffer holds a variety of data, all sent with the same\n>>> VC/DT (0x12).\n>>> * It comprises two scanlines (of up to 5760 bytes each, for 4608 pixels)\n>>> * of embedded data, one line of PDAF data, and two lines of AE-HIST data\n>>> * (AE histograms are valid for HDR mode and empty in non-HDR modes).\n>>> */\n>>> #define IMX708_EMBEDDED_LINE_WIDTH (5 * 5760)\n>>> #define IMX708_NUM_EMBEDDED_LINES 1\n>>>\n>>> \"Two scanlines of embedded data\" but macro IMX708_NUM_EMBEDDED_LINES = 1\n>>> ? Is scanlines different ?\n>> You should probably ignore this value. As you know we don't currently have\n>> official V4L2 support for embedded data or generic sensor metadata. For our\n>> downstream workaround this define is only used to size up the metadata buffer\n>> (since formats have to have a width and height in V4L2). It conveys no\n>> information on the actual embedded data structure.\n> The \"everything else\" channel in Unicam has no stride / bytesperline\n> configuration, therefore it will write the data exactly as it comes\n\nCan I ask why it is like that? Embedded data with same stride / bytes \nper line as the image area, makes sense to me. Using no stride probably \na try to optimise something ?\n> in. For most sensors the \"line length\" of the metadata will be the\n> same as that for the image mode, therefore the parser needs to know\n> the image format too.\n\nAh, I see.\n> The order that the sensor produces the lines of metadata is stated in\n> the datasheet, so the parser can know what to expect and in what\n> order.\n>\n> If a CSI2 receiver supports demultiplexing more data streams, and the\n> driver is amended to use different data id values, then the PDAF data\n> could be in a separate buffer to embedded data etc. So if you're\n> really trying to make it generic then you want to be passing in a\n> pointer to the start of the metadata.\n\nThat's is something i've locally (will be pushed shortly as RFC, have a \nlook).\n>\n> I would expect this to all change when Sakari lands the \"Generic line\n> based metadata support\" series that was discussed in Prague earlier\n> this week.\n>\n>>> \"One lines of PDAF data\" mentioned int the driver but libcamera-helper\n>>> [2] seems to consider two lines instead ?\n>>>\n>> The actual number of embedded data scan lines is 2.\n>>\n>>> Naushir, is there any specific documentation regarding the embedded data\n>>> / layout etc. ? It seems I am missing context.\n>> The datasheet for the imx708 is under strict NDA with Sony so unfortunately I\n>> cannot share any more than what is already in the parsing code here.\n>>\n>> I should also qualify that this is specific to the IMX708. I don't know\n>> anything about structure of the IMX519 metadata, but given the parsing code\n>> seems to give some sensible results, it may very well be the same.\n> Note that Arducam have just made a PR adding PDAF to the kernel driver\n> for their 64MPix camera (IMX682). They don't appear to have added PDAF\n> support to their libcamera fork[2], but I would expect it to happen\n> relatively soon. Perhaps it's worth waiting for that just to ensure\n> you're covering it too.\n\nAck. Till then let the patches iterate as RFC and have discussions \nongoing as well.\n>\n> Dave\n>\n> [1] https://github.com/raspberrypi/linux/pull/5523\n> [2] https://github.com/arducam/libcamera\n>\n>> Regards,\n>> Naush\n>>\n>>> [1]:\n>>> https://github.com/raspberrypi/linux/blob/rpi-6.4.y/drivers/media/i2c/imx708.c\n>>> [2]:\n>>> https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp#n127\n>>>>> + PdafRegions pdaf;\n>>>>> + parsePdafData(&buffer[2 * bytesPerLine],\n>>>>> + buffer.size() - 2 * bytesPerLine,\n>>>>> + mode_.bitdepth, pdaf);\n>>>>> + metadata.set(\"pdaf.data\", pdaf);\n>>>>> + }\n>>>>> +\n>>>>> /*\n>>>>> * The DeviceStatus struct is first populated with values obtained from\n>>>>> * DelayedControls. If this reports frame length is > frameLengthMax,\n>>>>> @@ -188,6 +210,34 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,\n>>>>> metadata.set(\"device.status\", deviceStatus);\n>>>>> }\n>>>>>\n>>>>> +bool CamHelperImx519::parsePdafData(const uint8_t *ptr, size_t len,\n>>>> This function should take a span instead of ptr + len.\n>>>>\n>>>>> + unsigned bpp, PdafRegions &pdaf)\n>>>>> +{\n>>>>> + size_t step = bpp >> 1; /* bytes per PDAF grid entry */\n>>>>> +\n>>>>> + if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {\n>>>> Can the bpp check fail, or do we always use RAW10 or RAW12 with this\n>>>> sensor ? The driver posted to the linux-media mailing list only supports\n>>>> RAW10. Does the sensor support RAW8 or RAW14 (or more) ?\n>>>>\n>>>> Where does the 194 come from ? A named constexpr, or a mathematical\n>>>> expression based on named constexprs, would be good too. Same for the\n>>>> first two bytes, magic values are not nice.\n>>>>\n>>>>> + LOG(IPARPI, Error) << \"PDAF data in unsupported format\";\n>>>>> + return false;\n>>>>> + }\n>>>>> +\n>>>>> + pdaf.init({ pdafStatsCols, pdafStatsRows });\n>>>>> +\n>>>>> + ptr += 2 * step;\n>>>>> + for (unsigned i = 0; i < pdafStatsRows; ++i) {\n>>>>> + for (unsigned j = 0; j < pdafStatsCols; ++j) {\n>>>>> + unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n>>>> uint8_t\n>>>>\n>>>>> + int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n>>>> Lowercase for hex constants.\n>>>>\n>>>>> + PdafData pdafData;\n>>>>> + pdafData.conf = c;\n>>>>> + pdafData.phase = c ? p : 0;\n>>>>> + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n>>>>> + ptr += step;\n>>>>> + }\n>>>>> + }\n>>>>> +\n>>>>> + return true;\n>>>>> +}\n>>>> Now that I've reviewed this, I realize all those commends apply to the\n>>>> IMX708 helper too. Can we factor this code out instead of duplicating it\n>>>> ?\n>>>>\n>>>>> +\n>>>>> static CamHelper *create()\n>>>>> {\n>>>>> return new CamHelperImx519();\n>>>>> diff --git a/src/ipa/rpi/vc4/data/imx519.json b/src/ipa/rpi/vc4/data/imx519.json\n>>>>> index 1b0a7747..0733d97e 100644\n>>>>> --- a/src/ipa/rpi/vc4/data/imx519.json\n>>>>> +++ b/src/ipa/rpi/vc4/data/imx519.json\n>>>>> @@ -350,6 +350,46 @@\n>>>>> ]\n>>>>> }\n>>>>> },\n>>>>> + {\n>>>>> + \"rpi.af\":\n>>>>> + {\n>>>>> + \"ranges\":\n>>>>> + {\n>>>>> + \"normal\":\n>>>>> + {\n>>>>> + \"min\": 0.0,\n>>>>> + \"max\": 12.0,\n>>>>> + \"default\": 1.0\n>>>>> + },\n>>>>> + \"macro\":\n>>>>> + {\n>>>>> + \"min\": 3.0,\n>>>>> + \"max\": 15.0,\n>>>>> + \"default\": 4.0\n>>>>> + }\n>>>>> + },\n>>>>> + \"speeds\":\n>>>>> + {\n>>>>> + \"normal\":\n>>>>> + {\n>>>>> + \"step_coarse\": 1.0,\n>>>>> + \"step_fine\": 0.25,\n>>>>> + \"contrast_ratio\": 0.75,\n>>>>> + \"pdaf_gain\": -0.02,\n>>>>> + \"pdaf_squelch\": 0.125,\n>>>>> + \"max_slew\": 2.0,\n>>>>> + \"pdaf_frames\": 20,\n>>>>> + \"dropout_frames\": 6,\n>>>>> + \"step_frames\": 4\n>>>>> + }\n>>>>> + },\n>>>>> + \"conf_epsilon\": 8,\n>>>>> + \"conf_thresh\": 16,\n>>>>> + \"conf_clip\": 512,\n>>>>> + \"skip_frames\": 5,\n>>>>> + \"map\": [ 0.0, 0.0, 15.0, 4095 ]\n>>>>> + }\n>>>>> + },\n>>>>> {\n>>>>> \"rpi.ccm\":\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 6F751BDB1D\n\tfor ;\n\tFri, 30 Jun 2023 11:39:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC57C60454;\n\tFri, 30 Jun 2023 13:39:42 +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 7AA5A60454\n\tfor ;\n\tFri, 30 Jun 2023 13:39:41 +0200 (CEST)","from [192.168.150.202] (unknown [193.85.242.128])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DDF0D4DE;\n\tFri, 30 Jun 2023 13:38:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688125182;\n\tbh=GL1KSDx3qDwCdoZOyXGgMvLayAhVPSuob9jcKD4NTcQ=;\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=uHzSKNuq6lfF2MzEOn8vySZLxLTbflPg+vMsZ/jbWS7k6Pxrfd1hp475/4EGm0Tov\n\tHPrx25ylP8gTlLrnPxQgF3cVO9Tu0DC0ImONNyHzpGzDJ29zRdDmE8iRB4MRM4Rokp\n\tRxOSRc5PHcXQHvetp7vXJXEgkVAltoGhnMeiQPyZC5jBmcISe4ZOvRdgzYsSH819LB\n\tLy+mxL2NldxadU9jVqhEVP0uaBIyIm6/e2xvQf5xHJDNu7iH/CEehp+kDZqn4W+Wuf\n\tNsVnf8jDpm3g573oSQVxU1NeIRkls/MFFBpqP6qF1eSo0/78nC4RWVFun9lbc4yNhO\n\tmdnhA8sKOaKFQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1688125140;\n\tbh=GL1KSDx3qDwCdoZOyXGgMvLayAhVPSuob9jcKD4NTcQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=XgTCbrgXAjZUnV/I131TfGfTGXtFt7Sb0maUd3W7YkAiwM34TR6GBfPQWOTi4bASn\n\tU7LEePBKwRFarcDykBXAU4bc/EQC84633s+U9QWQfq7kZ06brAseVcQbzwpDIUh0j5\n\teoUHYjB8Wcj16oWZm0TGYjSTQD0S2Gfw8zn5Fkx0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XgTCbrgX\"; dkim-atps=neutral","Message-ID":"<4c70957e-78be-1957-f8b2-2ba707d4135a@ideasonboard.com>","Date":"Fri, 30 Jun 2023 13:39:37 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","To":"Dave Stevenson ,\n\tNaushir Patuck ","References":"<20230622150015.45888-1-umang.jain@ideasonboard.com>\n\t<20230623234847.GC1669@pendragon.ideasonboard.com>\n\t\n\t\n\t","Content-Language":"en-US","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27446,"web_url":"https://patchwork.libcamera.org/comment/27446/","msgid":"","date":"2023-06-30T13:05:51","subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Umang\n\nOn Fri, 30 Jun 2023 at 12:39, Umang Jain wrote:\n>\n> Hi Dave, Naushir,\n>\n> On 6/30/23 12:55 PM, Dave Stevenson wrote:\n> > Hi Umang\n> >\n> > On Fri, 30 Jun 2023 at 08:53, Naushir Patuck via libcamera-devel\n> > wrote:\n> >> Hi Umang,\n> >>\n> >> On Thu, 29 Jun 2023 at 22:07, Umang Jain wrote:\n> >>> Hello,\n> >>>\n> >>> On 6/24/23 1:48 AM, Laurent Pinchart wrote:\n> >>>> Hi Umang,\n> >>>>\n> >>>> Thank you for the patch.\n> >>>>\n> >>>> CC'ing Naush, as a review from Raspberry Pi would be nice.\n> >>>>\n> >>>> On Thu, Jun 22, 2023 at 08:30:15PM +0530, Umang Jain via libcamera-devel wrote:\n> >>>>> From: Lee Jackson \n> >>>>>\n> >>>>> Adds the PDAF support for IMX519 camera module by Arducam.\n> >>>>>\n> >>>>> Tested-by: Umang Jain \n> >>>>> Signed-off-by: Umang Jain \n> >>>>> Signed-off-by: Lee Jackson \n> >>>>> ---\n> >>>>> Changes in v3:\n> >>>>> - Collect author's S-o-B from v2.\n> >>>>>\n> >>>>> ---\n> >>>>> src/ipa/rpi/cam_helper/cam_helper_imx519.cpp | 50 ++++++++++++++++++++\n> >>>>> src/ipa/rpi/vc4/data/imx519.json | 40 ++++++++++++++++\n> >>>>> 2 files changed, 90 insertions(+)\n> >>>>>\n> >>>>> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> >>>>> index c7262aa0..6d032082 100644\n> >>>>> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> >>>>> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> >>>>> @@ -15,9 +15,13 @@\n> >>>>>\n> >>>>> #include \n> >>>>>\n> >>>>> +#include \"controller/pdaf_data.h\"\n> >>>>> +\n> >>>>> #include \"cam_helper.h\"\n> >>>>> #include \"md_parser.h\"\n> >>>>>\n> >>>>> +#define ALIGN_UP(x, a) (((x) + (a)-1) & ~(a - 1))\n> >>>> utils.h has a alignUp() function.\n> >>>>\n> >>>>> +\n> >>>>> using namespace RPiController;\n> >>>>> using namespace libcamera;\n> >>>>> using libcamera::utils::Duration;\n> >>>>> @@ -66,8 +70,13 @@ private:\n> >>>>> /* Largest long exposure scale factor given as a left shift on the frame length. */\n> >>>>> static constexpr int longExposureShiftMax = 7;\n> >>>>>\n> >>>>> + static constexpr int pdafStatsRows = 12;\n> >>>>> + static constexpr int pdafStatsCols = 16;\n> >>>>> +\n> >>>>> void populateMetadata(const MdParser::RegisterMap ®isters,\n> >>>>> Metadata &metadata) const override;\n> >>>>> + static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> >>>>> + PdafRegions &pdaf);\n> >>>>> };\n> >>>>>\n> >>>>> CamHelperImx519::CamHelperImx519()\n> >>>>> @@ -90,6 +99,11 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> >>>>> MdParser::RegisterMap registers;\n> >>>>> DeviceStatus deviceStatus;\n> >>>>>\n> >>>>> + LOG(IPARPI, Debug) << \"Embedded buffer size: \" << buffer.size();\n> >>>>> +\n> >>>>> + size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n> >>>>> + bytesPerLine = ALIGN_UP(bytesPerLine, 16);\n> >>>>> +\n> >>>>> if (metadata.get(\"device.status\", deviceStatus)) {\n> >>>>> LOG(IPARPI, Error) << \"DeviceStatus not found from DelayedControls\";\n> >>>>> return;\n> >>>>> @@ -97,6 +111,14 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> >>>>>\n> >>>>> parseEmbeddedData(buffer, metadata);\n> >>>>>\n> >>>>> + if (buffer.size() > 2 * bytesPerLine) {\n> >>>> If my understanding is correct, the value 2 here means that the sensor\n> >>>> generates 2 lines of register embedded data, followed by PDAF data. A\n> >>>> name constexpr would be nice to replace the magic value. I would also\n> >>>> pass a subspan of 2 lines to the parseEmbeddedData() function, as it\n> >>>> shouldn't look into the PDAF section.\n> >>> I am refactoring this as mentioned in the review but having few questions?\n> >>>\n> >>> Is embedded data contains PDAF data as well ? OR embedded data is\n> >>> separate and PDAF data is separate sections ?\n> >> PDAF data comes after embedded data - typically with a different packet id.\n> >> However, because Unicam only has 2x DMA channels, embedded data, PDAF data and\n> >> HDR histogram data all get routed into a single metadata buffer.\n> > For IMX708 the CSI2 data type for additional metadata lines (PDAF, AE,\n> > histograms, etc) is configurable, but our downstream driver sets them\n> > to use 0x12 to be the same as the embedded data. As Naush says, it\n> > makes no difference with Unicam as that only has 2 data paths (image,\n> > and \"everything else\"), but keeps it a little cleaner.\n> >\n> >>> For e.g. I was looking at IMX708 driver [1], and what it states is\n> >>> confusing to me as well:\n> >>>\n> >>> /*\n> >>> * Metadata buffer holds a variety of data, all sent with the same\n> >>> VC/DT (0x12).\n> >>> * It comprises two scanlines (of up to 5760 bytes each, for 4608 pixels)\n> >>> * of embedded data, one line of PDAF data, and two lines of AE-HIST data\n> >>> * (AE histograms are valid for HDR mode and empty in non-HDR modes).\n> >>> */\n> >>> #define IMX708_EMBEDDED_LINE_WIDTH (5 * 5760)\n> >>> #define IMX708_NUM_EMBEDDED_LINES 1\n> >>>\n> >>> \"Two scanlines of embedded data\" but macro IMX708_NUM_EMBEDDED_LINES = 1\n> >>> ? Is scanlines different ?\n> >> You should probably ignore this value. As you know we don't currently have\n> >> official V4L2 support for embedded data or generic sensor metadata. For our\n> >> downstream workaround this define is only used to size up the metadata buffer\n> >> (since formats have to have a width and height in V4L2). It conveys no\n> >> information on the actual embedded data structure.\n> > The \"everything else\" channel in Unicam has no stride / bytesperline\n> > configuration, therefore it will write the data exactly as it comes\n>\n> Can I ask why it is like that? Embedded data with same stride / bytes\n> per line as the image area, makes sense to me. Using no stride probably\n> a try to optimise something ?\n\nThere's no requirement for alternate data types to have specific\nlengths per frame.\n\nTaking the Toshiba TC358743 HDMI to CSI2 bridge chip as an example, it\ncan pass HDMI audio data and InfoFrames over CSI2 using alternate data\ntypes. That's using the same virtual channel as the images, but audio\nis largely independent of any framing, let alone image parameters. How\nwould you handle that in the receiver if you forced a stride type\nparameter on it?\n\n> > in. For most sensors the \"line length\" of the metadata will be the\n> > same as that for the image mode, therefore the parser needs to know\n> > the image format too.\n>\n> Ah, I see.\n\nThe IMX219 datasheet is public (although not officially) at [1]. Fig\n20 \"Frame Structure for Serial signal output\" on page 47 gives you an\nexample of the typical framing definition you'll get for a sensor. The\nPacket Header on each line dictates what the line contains. The\nembedded data lines will use data type 0x12 in their header, whilst\nthe image data will be 0x2a for raw8, or 0x2b for raw10 (specified in\nthe MIPI CSI-2 spec).\nThe generic case is presented in the CSI-2 spec section 11.1.2\n\"Embedded Information\", and Figure 57 \"Frame Structure with Embedded\nData at the Beginning and End of the Frame\".\n\nFor IMX708 (and presumably the others) you'll find that it adds a\nnumber of lines afterwards in a specific order for all the extra\nmetadata. Registers allow enabling/disabling of each, and setting the\ndata type field to be used.\n\nThe MIPI CCS spec [2] contains the packing of the registers into the\nembedded buffer - section 7.15 \"Embedded Data Line Formats\" in v1.1 (2\nDec 2019). Most sensors seem to follow that, even if they aren't\ncompliant to CCS with regard to configuration.\n\n Dave\n\n[1] https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS/blob/master/RASPBERRY%20PI%20CAMERA%20V2%20DATASHEET%20IMX219PQH5_7.0.0_Datasheet_XXX.PDF\n[2] https://www.mipi.org/camera-command-set-v1-1-1-download\n\nSorry, the CSI-2 is restricted to MIPI Alliance members only, but\nthere are copies floating about on the web. I'm not going to directly\nlink to any. Very little changes between versions, so v1.01 April 2009\nis sufficient.\n\n> > The order that the sensor produces the lines of metadata is stated in\n> > the datasheet, so the parser can know what to expect and in what\n> > order.\n> >\n> > If a CSI2 receiver supports demultiplexing more data streams, and the\n> > driver is amended to use different data id values, then the PDAF data\n> > could be in a separate buffer to embedded data etc. So if you're\n> > really trying to make it generic then you want to be passing in a\n> > pointer to the start of the metadata.\n>\n> That's is something i've locally (will be pushed shortly as RFC, have a\n> look).\n> >\n> > I would expect this to all change when Sakari lands the \"Generic line\n> > based metadata support\" series that was discussed in Prague earlier\n> > this week.\n> >\n> >>> \"One lines of PDAF data\" mentioned int the driver but libcamera-helper\n> >>> [2] seems to consider two lines instead ?\n> >>>\n> >> The actual number of embedded data scan lines is 2.\n> >>\n> >>> Naushir, is there any specific documentation regarding the embedded data\n> >>> / layout etc. ? It seems I am missing context.\n> >> The datasheet for the imx708 is under strict NDA with Sony so unfortunately I\n> >> cannot share any more than what is already in the parsing code here.\n> >>\n> >> I should also qualify that this is specific to the IMX708. I don't know\n> >> anything about structure of the IMX519 metadata, but given the parsing code\n> >> seems to give some sensible results, it may very well be the same.\n> > Note that Arducam have just made a PR adding PDAF to the kernel driver\n> > for their 64MPix camera (IMX682). They don't appear to have added PDAF\n> > support to their libcamera fork[2], but I would expect it to happen\n> > relatively soon. Perhaps it's worth waiting for that just to ensure\n> > you're covering it too.\n>\n> Ack. Till then let the patches iterate as RFC and have discussions\n> ongoing as well.\n> >\n> > Dave\n> >\n> > [1] https://github.com/raspberrypi/linux/pull/5523\n> > [2] https://github.com/arducam/libcamera\n> >\n> >> Regards,\n> >> Naush\n> >>\n> >>> [1]:\n> >>> https://github.com/raspberrypi/linux/blob/rpi-6.4.y/drivers/media/i2c/imx708.c\n> >>> [2]:\n> >>> https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp#n127\n> >>>>> + PdafRegions pdaf;\n> >>>>> + parsePdafData(&buffer[2 * bytesPerLine],\n> >>>>> + buffer.size() - 2 * bytesPerLine,\n> >>>>> + mode_.bitdepth, pdaf);\n> >>>>> + metadata.set(\"pdaf.data\", pdaf);\n> >>>>> + }\n> >>>>> +\n> >>>>> /*\n> >>>>> * The DeviceStatus struct is first populated with values obtained from\n> >>>>> * DelayedControls. If this reports frame length is > frameLengthMax,\n> >>>>> @@ -188,6 +210,34 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,\n> >>>>> metadata.set(\"device.status\", deviceStatus);\n> >>>>> }\n> >>>>>\n> >>>>> +bool CamHelperImx519::parsePdafData(const uint8_t *ptr, size_t len,\n> >>>> This function should take a span instead of ptr + len.\n> >>>>\n> >>>>> + unsigned bpp, PdafRegions &pdaf)\n> >>>>> +{\n> >>>>> + size_t step = bpp >> 1; /* bytes per PDAF grid entry */\n> >>>>> +\n> >>>>> + if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {\n> >>>> Can the bpp check fail, or do we always use RAW10 or RAW12 with this\n> >>>> sensor ? The driver posted to the linux-media mailing list only supports\n> >>>> RAW10. Does the sensor support RAW8 or RAW14 (or more) ?\n> >>>>\n> >>>> Where does the 194 come from ? A named constexpr, or a mathematical\n> >>>> expression based on named constexprs, would be good too. Same for the\n> >>>> first two bytes, magic values are not nice.\n> >>>>\n> >>>>> + LOG(IPARPI, Error) << \"PDAF data in unsupported format\";\n> >>>>> + return false;\n> >>>>> + }\n> >>>>> +\n> >>>>> + pdaf.init({ pdafStatsCols, pdafStatsRows });\n> >>>>> +\n> >>>>> + ptr += 2 * step;\n> >>>>> + for (unsigned i = 0; i < pdafStatsRows; ++i) {\n> >>>>> + for (unsigned j = 0; j < pdafStatsCols; ++j) {\n> >>>>> + unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> >>>> uint8_t\n> >>>>\n> >>>>> + int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> >>>> Lowercase for hex constants.\n> >>>>\n> >>>>> + PdafData pdafData;\n> >>>>> + pdafData.conf = c;\n> >>>>> + pdafData.phase = c ? p : 0;\n> >>>>> + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> >>>>> + ptr += step;\n> >>>>> + }\n> >>>>> + }\n> >>>>> +\n> >>>>> + return true;\n> >>>>> +}\n> >>>> Now that I've reviewed this, I realize all those commends apply to the\n> >>>> IMX708 helper too. Can we factor this code out instead of duplicating it\n> >>>> ?\n> >>>>\n> >>>>> +\n> >>>>> static CamHelper *create()\n> >>>>> {\n> >>>>> return new CamHelperImx519();\n> >>>>> diff --git a/src/ipa/rpi/vc4/data/imx519.json b/src/ipa/rpi/vc4/data/imx519.json\n> >>>>> index 1b0a7747..0733d97e 100644\n> >>>>> --- a/src/ipa/rpi/vc4/data/imx519.json\n> >>>>> +++ b/src/ipa/rpi/vc4/data/imx519.json\n> >>>>> @@ -350,6 +350,46 @@\n> >>>>> ]\n> >>>>> }\n> >>>>> },\n> >>>>> + {\n> >>>>> + \"rpi.af\":\n> >>>>> + {\n> >>>>> + \"ranges\":\n> >>>>> + {\n> >>>>> + \"normal\":\n> >>>>> + {\n> >>>>> + \"min\": 0.0,\n> >>>>> + \"max\": 12.0,\n> >>>>> + \"default\": 1.0\n> >>>>> + },\n> >>>>> + \"macro\":\n> >>>>> + {\n> >>>>> + \"min\": 3.0,\n> >>>>> + \"max\": 15.0,\n> >>>>> + \"default\": 4.0\n> >>>>> + }\n> >>>>> + },\n> >>>>> + \"speeds\":\n> >>>>> + {\n> >>>>> + \"normal\":\n> >>>>> + {\n> >>>>> + \"step_coarse\": 1.0,\n> >>>>> + \"step_fine\": 0.25,\n> >>>>> + \"contrast_ratio\": 0.75,\n> >>>>> + \"pdaf_gain\": -0.02,\n> >>>>> + \"pdaf_squelch\": 0.125,\n> >>>>> + \"max_slew\": 2.0,\n> >>>>> + \"pdaf_frames\": 20,\n> >>>>> + \"dropout_frames\": 6,\n> >>>>> + \"step_frames\": 4\n> >>>>> + }\n> >>>>> + },\n> >>>>> + \"conf_epsilon\": 8,\n> >>>>> + \"conf_thresh\": 16,\n> >>>>> + \"conf_clip\": 512,\n> >>>>> + \"skip_frames\": 5,\n> >>>>> + \"map\": [ 0.0, 0.0, 15.0, 4095 ]\n> >>>>> + }\n> >>>>> + },\n> >>>>> {\n> >>>>> \"rpi.ccm\":\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 298BFBE175\n\tfor ;\n\tFri, 30 Jun 2023 13:06:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D1E2628C1;\n\tFri, 30 Jun 2023 15:06:11 +0200 (CEST)","from mail-yb1-xb2f.google.com (mail-yb1-xb2f.google.com\n\t[IPv6:2607:f8b0:4864:20::b2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F8CE628BD\n\tfor ;\n\tFri, 30 Jun 2023 15:06:10 +0200 (CEST)","by mail-yb1-xb2f.google.com with SMTP id\n\t3f1490d57ef6-c2cf4e61bc6so1800456276.3\n\tfor ;\n\tFri, 30 Jun 2023 06:06:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688130371;\n\tbh=2qYKmrXPktGaiwuUuruO3yfVEvAHHWLl5kXZMd7HxlY=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yyyW4N20mpUL0aCaitNG0rgQebs23SPrfm+HVa6l3hbJZC6B+HhJ2kxy31pIrxb4I\n\tUXlHbedyojZc7nv56oyrij6qx2ozB4eLaXKBjF0svHhcru/rELHsTv54WnnWvxcWda\n\tWNoq8AJgZDei1oGiMqgAWrDQ+ViorPnXtPQMpKa1xjRfJ3xsQ+cZ05BqUT+KQlYtJb\n\t5ZTRgddXyL2YXj+GXNseXXoeiSsvTvT2aoOwUXyOCOykaXERv4WkqEUo9b9xAQsyyB\n\tg722MVnE4D19V/1MIfD4tHgdfAbxcW3thFHWscCwh6+K6Shw5JaRGf2bAXZ3b0obUJ\n\t/lYFHvaAJQGNg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1688130369; x=1690722369;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=1w1NiLdYyrY4BZnaziR3bG1CJP4BzpdlDF+xpe7ztdU=;\n\tb=tmY5rQ/jp8CgEKme2a0/KpVb4dPfdZ1rx3IR19+0PXFIufm9umg9xTHPfCDzI8Pe9V\n\tuWYWVR6bbUJVdQQJwtX46BFmbL6r77445OElNaBwMJfs3LWyFX60BTaur0leQ7a3pbhw\n\tbUeO71W6cQJEx5dQIsQXOuRCwXM1aswnXHS02e8Sk026VZVgrSCHiLIY/YZTI7VlOl9c\n\tIrzSq1wChwIeTM/05MMcDZT7QJ2PmVMPU/yfKfeqFgBh3LI928dxSq1GCvxfTwglOsrE\n\tTILB8Sy4lVwg2Hu4qYHkxljQf68WVnbTV/qoDXVml3ZqoTdcaO+79OOTTh45yDSjzej6\n\t1DWw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"tmY5rQ/j\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1688130369; x=1690722369;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=1w1NiLdYyrY4BZnaziR3bG1CJP4BzpdlDF+xpe7ztdU=;\n\tb=L2qoJfvIXLfxjXgYfH6vPcT0bV2bINevZ8ipdPFk3T43OVQtkrZJRkPzs1OeFX+Xgk\n\tEHNDLZc0v237KSnitg0tId4bKZajxh099k6qsfVlRIi28M1UEbPZ+/QzzKhl4/kRVQKa\n\ts5QTHR3gtRPHaYmA6qrLCvAOuuoQmoDSOH3uHI+KYMQF6e8h70yDP9WTNPGFaPOdA+5g\n\tc+ixWua32CV50melqFh/+CkusGTmMA40YNCFkVXZhnVE/gluAPOHKw8XgqLt42Hvbzxh\n\tCV9Wk9pxyUnE0z6Sck5HE9/nKoaDbP+sdPgAcyyCsm5J0/dOWuX2SU/QGnNw5qWwuUnL\n\tloXw==","X-Gm-Message-State":"ABy/qLYwSnWDFMivu9LmatTlKPWTn4IBBPlJc/AxzSKRIOPltqnbSL3D\n\tSkmWwy7WpcRZdgoLvue+OJOLbbtLkhYi6n5pMuf8Mg==","X-Google-Smtp-Source":"APBJJlH0MUBgdUD9YLY4ohutF2yPnzpO+TQ0tHAMiIOXQGnx7Db6yw+OYV5HtlyNazvXSj8XPLgjWYRgaTeaLXjLrfA=","X-Received":"by 2002:a25:e6d6:0:b0:b9a:7df5:4f26 with SMTP id\n\td205-20020a25e6d6000000b00b9a7df54f26mr1625299ybh.30.1688130367700;\n\tFri, 30 Jun 2023 06:06:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20230622150015.45888-1-umang.jain@ideasonboard.com>\n\t<20230623234847.GC1669@pendragon.ideasonboard.com>\n\t\n\t\n\t\n\t<4c70957e-78be-1957-f8b2-2ba707d4135a@ideasonboard.com>","In-Reply-To":"<4c70957e-78be-1957-f8b2-2ba707d4135a@ideasonboard.com>","Date":"Fri, 30 Jun 2023 14:05:51 +0100","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support\n\tfor IMX519","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":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]