[{"id":15237,"web_url":"https://patchwork.libcamera.org/comment/15237/","msgid":"","date":"2021-02-19T13:46:02","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 19/02/2021 11:22, Jacopo Mondi wrote:\n> When a Request is completed upon receiving the IPA produced metadata,\n> the metadata associated with the Request are over-written, deleting\n> the information set, in example, at ImgU output buffer completion time.\n> \n> If any additional Request metadata should be registered by inspecting\n> the IPA produced metadata it has to be done without deleting the already\n> registered entries.\n> \n> Fix this by replacing the metadata over-write with a todo entry.\n> \n> This change fixes CTS which was broken due to missing metadata in\n> the completed requests.\n> \n> Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2aed826a892a..9e867ab2e98a 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> \t\tif (!info)\n> \t\t\tbreak;\n> \n> +\t\t/*\n> +\t\t * \\todo Parse the value of the controls returned by the IPA\n> +\t\t * in action.controls to register additional request metadata.\n> +\t\t */\n> \t\tRequest *request = info->request;\n> -\t\trequest->metadata() = action.controls;\n\nI'm wondering if our objects let us do metadata() += controls; here, but\nI only as a query if we can update control lists with anther list,\nrathar than overwrite it, which is perhaps what was inteneded here.\n\nBut even still - it needs full consideration, so it's better removed\nuntil it's actually needed and used.\n\nReviewed-by: Kieran Bingham \n\n> \t\tinfo->metadataProcessed = true;\n> \t\tif (frameInfos_.tryComplete(info))\n> \t\t\tpipe_->completeRequest(request);\n> --\n> 2.30.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 2E295BD1F1\n\tfor ;\n\tFri, 19 Feb 2021 13:46:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8B14689C7;\n\tFri, 19 Feb 2021 14:46:08 +0100 (CET)","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 67A22602F5\n\tfor ;\n\tFri, 19 Feb 2021 14:46:06 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CA3CE596;\n\tFri, 19 Feb 2021 14:46:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"olEUaYjn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613742366;\n\tbh=OnleAXg5Z0v8yOqaEAIHdNWqT+jyRLVuG0IKmc5l81U=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=olEUaYjnpoNInpu7F6Nk1s2yRQip/CbNr8UEs0ADouNFe51QmY5hV6+Wow36IngYj\n\tNHvlZbwVM8pMY1/IdSqJUbojn8mXtft8ayYOolwixe6CIzVfbr5fyYBQDrRfkwtQNI\n\tN30Vwz264Flyg/TMR58G2ESjQy9hTbWyBkgp8wXY=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210219112257.53307-1-jacopo@jmondi.org>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 19 Feb 2021 13:46:02 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210219112257.53307-1-jacopo@jmondi.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15241,"web_url":"https://patchwork.libcamera.org/comment/15241/","msgid":"","date":"2021-02-19T14:28:34","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:\n> When a Request is completed upon receiving the IPA produced metadata,\n> the metadata associated with the Request are over-written, deleting\n> the information set, in example, at ImgU output buffer completion time.\n> \n> If any additional Request metadata should be registered by inspecting\n> the IPA produced metadata it has to be done without deleting the already\n> registered entries.\n> \n> Fix this by replacing the metadata over-write with a todo entry.\n> \n> This change fixes CTS which was broken due to missing metadata in\n> the completed requests.\n> \n> Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2aed826a892a..9e867ab2e98a 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> \t\tif (!info)\n> \t\t\tbreak;\n> \n> +\t\t/*\n> +\t\t * \\todo Parse the value of the controls returned by the IPA\n> +\t\t * in action.controls to register additional request metadata.\n> +\t\t */\n> \t\tRequest *request = info->request;\n> -\t\trequest->metadata() = action.controls;\n\nI understand the intent is to keep metadata set by the pipeline but this \nchange simply moves the problem. With this change any metadata reported \nby the IPA is discarded. I thought the idea was that all metadata would \nbe generated by the IPA? If not I think we need to merge the two here.\n\nIf we wish to take this in the direction of this patch I think the IPA \ninterface and IPA should also be updated to not report any metadata.\n\n> \t\tinfo->metadataProcessed = true;\n> \t\tif (frameInfos_.tryComplete(info))\n> \t\t\tpipe_->completeRequest(request);\n> --\n> 2.30.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 B74D9BD1F1\n\tfor ;\n\tFri, 19 Feb 2021 14:28:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 504C5602F5;\n\tFri, 19 Feb 2021 15:28:40 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9691602F5\n\tfor ;\n\tFri, 19 Feb 2021 15:28:38 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id e17so21466239ljl.8\n\tfor ;\n\tFri, 19 Feb 2021 06:28:38 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\td14sm940421lfg.128.2021.02.19.06.28.35\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 19 Feb 2021 06:28:35 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"x6hxQZY6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=/D9+M4SMtF2JrX9tJtLOjHrMqVegaDn9eaHKfbjMTCs=;\n\tb=x6hxQZY6Be5YC1RKgWQPFQZJJ+Lf8K1B1HFISw/imB4hZlczulS0coXxpqMIASZOLc\n\tJE/g5Puxe1PHlT6f6l6jb84v+Gd6SpppE+hVDyxl1WwrVUd1s7hUd5kbaoMJJ8rB5ACe\n\ttxDjs2K6wuHjI0p98OuB7ib134fRI3Tx7HizBKb9qd2shpljZLh2NRaTmMC09myExp3a\n\tYbaSs8RL5yvgQGNoEtEWb30map/PXCkUvxNIU8jI2+sOg1gjoi0PiMVhgJT7XR3TRgt1\n\tDYrI+tROsywWfESduPOznNUJXIFm2Yh2uE/IQkVPEOHQSFbhg4Td57gO4hbyOEA/clN8\n\tRaSg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=/D9+M4SMtF2JrX9tJtLOjHrMqVegaDn9eaHKfbjMTCs=;\n\tb=Wv47hJZWXvrRZioqiaqdP5J4N84WBiKUfdPxBMLNEAtFeR1bjf++GPG59aPwKAD31I\n\tYmfPUzzbrLqigyjIIrTTQVZ6seKEgV24dHBgdSjX+/ZQ4lSfsn6CnJL3PqhC+dMduCpD\n\tG8e0zl7w6kKj92SPGAHzd8mpoBXw3LJzDp9jlu5KaPDzt7qcRPbV6CukzNrI42eDuL18\n\tAlpa+6sf1O+GkAx6Fu6zGHoL93BE7oqQd8uacirz/I0VvbyHPiq2cxIoFvPuqMANFjzQ\n\tkxKW9czjLO1oUG9TwO8+stX1/F3e/izIzRPstOQsv6GECUe/5Mqu4bl3DpcX8rUlVb9L\n\tQ87Q==","X-Gm-Message-State":"AOAM533UdOtvzz8HcBlKSeAHV14cTlZcqzajEnNH7qtM/vSHPW8PgrFx\n\tDNJiAZCkyvxUKpX+WERWg2ExyKCzsXM0hHb/","X-Google-Smtp-Source":"ABdhPJwZImI8T50ZcJEkN3kiXSJhdXqXIHWDMeDp4c/5pcUkG+RcgBa8gqjtisRNbBuWeyrioUmOSQ==","X-Received":"by 2002:a2e:9655:: with SMTP id\n\tz21mr5723141ljh.486.1613744916806; \n\tFri, 19 Feb 2021 06:28:36 -0800 (PST)","Date":"Fri, 19 Feb 2021 15:28:34 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210219112257.53307-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210219112257.53307-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15258,"web_url":"https://patchwork.libcamera.org/comment/15258/","msgid":"","date":"2021-02-21T18:39:52","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:\n> On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:\n> > When a Request is completed upon receiving the IPA produced metadata,\n> > the metadata associated with the Request are over-written, deleting\n> > the information set, in example, at ImgU output buffer completion time.\n> > \n> > If any additional Request metadata should be registered by inspecting\n> > the IPA produced metadata it has to be done without deleting the already\n> > registered entries.\n> > \n> > Fix this by replacing the metadata over-write with a todo entry.\n> > \n> > This change fixes CTS which was broken due to missing metadata in\n> > the completed requests.\n> > \n> > Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 2aed826a892a..9e867ab2e98a 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> > \t\tif (!info)\n> > \t\t\tbreak;\n> > \n> > +\t\t/*\n> > +\t\t * \\todo Parse the value of the controls returned by the IPA\n> > +\t\t * in action.controls to register additional request metadata.\n> > +\t\t */\n> > \t\tRequest *request = info->request;\n> > -\t\trequest->metadata() = action.controls;\n> \n> I understand the intent is to keep metadata set by the pipeline but this \n> change simply moves the problem. With this change any metadata reported \n> by the IPA is discarded. I thought the idea was that all metadata would \n> be generated by the IPA? If not I think we need to merge the two here.\n\nMerging the two seems like the right solution. The ControlList class\ndoesn't support this at the moment, but it shouldn't be difficult to\nextend it. A merge() function, along the lines of\nhttps://en.cppreference.com/w/cpp/container/unordered_map/merge, should\ndo.\n\n> If we wish to take this in the direction of this patch I think the IPA \n> interface and IPA should also be updated to not report any metadata.\n> \n> > \t\tinfo->metadataProcessed = true;\n> > \t\tif (frameInfos_.tryComplete(info))\n> > \t\t\tpipe_->completeRequest(request);","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 E69F5BD1F1\n\tfor ;\n\tSun, 21 Feb 2021 18:40:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 577B0689E1;\n\tSun, 21 Feb 2021 19:40:21 +0100 (CET)","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 338BD689C2\n\tfor ;\n\tSun, 21 Feb 2021 19:40:19 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89700EF;\n\tSun, 21 Feb 2021 19:40:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Bj/u+a96\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613932818;\n\tbh=d0Lx5OwAY033WB3B06UB6qEEn+4MswG1kPzCgxxtt0g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Bj/u+a960lWptNWftyaeKPDrqbhMNQprod7WkhP3VrSCaiqU7zFjJCHY/8iUxhu4E\n\trlGV/Vfg9kQnEs7p1GC7dPyI3Xp+EloHX50fbYERpwmMTVjDERCs6uP11iHZeGpPs8\n\t3DCnFWLaRAA4n1qWxCBfop8xxO7KtN1TvesU4Nqc=","Date":"Sun, 21 Feb 2021 20:39:52 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210219112257.53307-1-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15274,"web_url":"https://patchwork.libcamera.org/comment/15274/","msgid":"<20210222090108.c4ifaau5rzq5fd4n@uno.localdomain>","date":"2021-02-22T09:01:08","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello Niklas, Laurent,\n\nOn Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:\n> Hello,\n>\n> On Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:\n> > On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:\n> > > When a Request is completed upon receiving the IPA produced metadata,\n> > > the metadata associated with the Request are over-written, deleting\n> > > the information set, in example, at ImgU output buffer completion time.\n> > >\n> > > If any additional Request metadata should be registered by inspecting\n> > > the IPA produced metadata it has to be done without deleting the already\n> > > registered entries.\n> > >\n> > > Fix this by replacing the metadata over-write with a todo entry.\n> > >\n> > > This change fixes CTS which was broken due to missing metadata in\n> > > the completed requests.\n> > >\n> > > Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 2aed826a892a..9e867ab2e98a 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> > > \t\tif (!info)\n> > > \t\t\tbreak;\n> > >\n> > > +\t\t/*\n> > > +\t\t * \\todo Parse the value of the controls returned by the IPA\n> > > +\t\t * in action.controls to register additional request metadata.\n> > > +\t\t */\n> > > \t\tRequest *request = info->request;\n> > > -\t\trequest->metadata() = action.controls;\n> >\n> > I understand the intent is to keep metadata set by the pipeline but this\n> > change simply moves the problem. With this change any metadata reported\n> > by the IPA is discarded. I thought the idea was that all metadata would\n> > be generated by the IPA? If not I think we need to merge the two here.\n>\n> Merging the two seems like the right solution. The ControlList class\n> doesn't support this at the moment, but it shouldn't be difficult to\n> extend it. A merge() function, along the lines of\n> https://en.cppreference.com/w/cpp/container/unordered_map/merge, should\n> do.\n>\n\nAt the moment the IPA does not report any metadata if I'm not\nmistaken, and this change breaks 80+ CTS tests as it overwrites the actual\nmetadata with an empty list.\n\nI would also question that the pipeline handler should blindly merge\nthe two as over-writes might happen, but I might be mistaken.\n\nAnyway, I'm fine having more CTS tests broken waiting for the IPA to\nactually set something there if that's the desired direction. I would\nprefer the other way around as this hinders CTS progresses but I would\ngo with what the group prefers.\n\n> > If we wish to take this in the direction of this patch I think the IPA\n> > interface and IPA should also be updated to not report any metadata.\n> >\n> > > \t\tinfo->metadataProcessed = true;\n> > > \t\tif (frameInfos_.tryComplete(info))\n> > > \t\t\tpipe_->completeRequest(request);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 A5D4ABD1F6\n\tfor ;\n\tMon, 22 Feb 2021 09:00:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33996689FA;\n\tMon, 22 Feb 2021 10:00:44 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7128B602F3\n\tfor ;\n\tMon, 22 Feb 2021 10:00:42 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 658E940007;\n\tMon, 22 Feb 2021 09:00:40 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 22 Feb 2021 10:01:08 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210222090108.c4ifaau5rzq5fd4n@uno.localdomain>","References":"<20210219112257.53307-1-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15275,"web_url":"https://patchwork.libcamera.org/comment/15275/","msgid":"","date":"2021-02-22T09:18:51","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Feb 22, 2021 at 10:01:08AM +0100, Jacopo Mondi wrote:\n> On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:\n> > On Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:\n> > > On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:\n> > > > When a Request is completed upon receiving the IPA produced metadata,\n> > > > the metadata associated with the Request are over-written, deleting\n> > > > the information set, in example, at ImgU output buffer completion time.\n> > > >\n> > > > If any additional Request metadata should be registered by inspecting\n> > > > the IPA produced metadata it has to be done without deleting the already\n> > > > registered entries.\n> > > >\n> > > > Fix this by replacing the metadata over-write with a todo entry.\n> > > >\n> > > > This change fixes CTS which was broken due to missing metadata in\n> > > > the completed requests.\n> > > >\n> > > > Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> > > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 2aed826a892a..9e867ab2e98a 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> > > > \t\tif (!info)\n> > > > \t\t\tbreak;\n> > > >\n> > > > +\t\t/*\n> > > > +\t\t * \\todo Parse the value of the controls returned by the IPA\n> > > > +\t\t * in action.controls to register additional request metadata.\n> > > > +\t\t */\n> > > > \t\tRequest *request = info->request;\n> > > > -\t\trequest->metadata() = action.controls;\n> > >\n> > > I understand the intent is to keep metadata set by the pipeline but this\n> > > change simply moves the problem. With this change any metadata reported\n> > > by the IPA is discarded. I thought the idea was that all metadata would\n> > > be generated by the IPA? If not I think we need to merge the two here.\n> >\n> > Merging the two seems like the right solution. The ControlList class\n> > doesn't support this at the moment, but it shouldn't be difficult to\n> > extend it. A merge() function, along the lines of\n> > https://en.cppreference.com/w/cpp/container/unordered_map/merge, should\n> > do.\n> \n> At the moment the IPA does not report any metadata if I'm not\n> mistaken, and this change breaks 80+ CTS tests as it overwrites the actual\n> metadata with an empty list.\n\nCorrect, so I'm fine with this patch, but we'll need to immediately\nimprove it on top :-)\n\n> I would also question that the pipeline handler should blindly merge\n> the two as over-writes might happen, but I might be mistaken.\n\nIn theory that's possible, but given that the pipeline handler and IPA\nare supposed to be developed in sync, that's something the developer\nshould be able to control. They could either make sure not conflict\noccurs, or implement a more elaborate merge method. I would expect the\nformer to be the common case, hence the proposal for\nControlList::merge().\n\n> Anyway, I'm fine having more CTS tests broken waiting for the IPA to\n> actually set something there if that's the desired direction. I would\n> prefer the other way around as this hinders CTS progresses but I would\n> go with what the group prefers.\n\nNo, we can merge this patch first, that's not an issue. The fact that\nNiklas and I have pointed out issues doesn't mean they all have to be\nfixed as a prerequisite, they can also go on top where it makes sense\n:-)\n\n> > > If we wish to take this in the direction of this patch I think the IPA\n> > > interface and IPA should also be updated to not report any metadata.\n> > >\n> > > > \t\tinfo->metadataProcessed = true;\n> > > > \t\tif (frameInfos_.tryComplete(info))\n> > > > \t\t\tpipe_->completeRequest(request);","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 4C11CBD1F1\n\tfor ;\n\tMon, 22 Feb 2021 09:19:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3E1B689FB;\n\tMon, 22 Feb 2021 10:19:19 +0100 (CET)","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 67590602F3\n\tfor ;\n\tMon, 22 Feb 2021 10:19:18 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C88C2344;\n\tMon, 22 Feb 2021 10:19:17 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OwlYamZL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613985558;\n\tbh=FnwIkScR7yMuSp026LIodPMAbkQdXEu25Hz8zgR+ry4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OwlYamZL2czrVMKGAo79dKSZ5lq1aYhmLUMQFOWHPBn/J1qtNX6RWXzQ4/5L26BC2\n\tYBrQlTCdllfU3jvchqarMdgHCsIsTKu4mkVk7HmQyHzv0jA6YVMutYjzaOP/Y9wKe/\n\tiyWpYlleGYNez3KyaFd/kxVCUhTwtl4BA56irREk=","Date":"Mon, 22 Feb 2021 11:18:51 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210219112257.53307-1-jacopo@jmondi.org>\n\t\n\t\n\t<20210222090108.c4ifaau5rzq5fd4n@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210222090108.c4ifaau5rzq5fd4n@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15281,"web_url":"https://patchwork.libcamera.org/comment/15281/","msgid":"<20210222102441.xq27dckghjqqosnd@uno.localdomain>","date":"2021-02-22T10:24:41","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Feb 22, 2021 at 11:18:51AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Feb 22, 2021 at 10:01:08AM +0100, Jacopo Mondi wrote:\n> > On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:\n> > > On Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:\n> > > > On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:\n> > > > > When a Request is completed upon receiving the IPA produced metadata,\n> > > > > the metadata associated with the Request are over-written, deleting\n> > > > > the information set, in example, at ImgU output buffer completion time.\n> > > > >\n> > > > > If any additional Request metadata should be registered by inspecting\n> > > > > the IPA produced metadata it has to be done without deleting the already\n> > > > > registered entries.\n> > > > >\n> > > > > Fix this by replacing the metadata over-write with a todo entry.\n> > > > >\n> > > > > This change fixes CTS which was broken due to missing metadata in\n> > > > > the completed requests.\n> > > > >\n> > > > > Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > index 2aed826a892a..9e867ab2e98a 100644\n> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> > > > > \t\tif (!info)\n> > > > > \t\t\tbreak;\n> > > > >\n> > > > > +\t\t/*\n> > > > > +\t\t * \\todo Parse the value of the controls returned by the IPA\n> > > > > +\t\t * in action.controls to register additional request metadata.\n> > > > > +\t\t */\n> > > > > \t\tRequest *request = info->request;\n> > > > > -\t\trequest->metadata() = action.controls;\n> > > >\n> > > > I understand the intent is to keep metadata set by the pipeline but this\n> > > > change simply moves the problem. With this change any metadata reported\n> > > > by the IPA is discarded. I thought the idea was that all metadata would\n> > > > be generated by the IPA? If not I think we need to merge the two here.\n> > >\n> > > Merging the two seems like the right solution. The ControlList class\n> > > doesn't support this at the moment, but it shouldn't be difficult to\n> > > extend it. A merge() function, along the lines of\n> > > https://en.cppreference.com/w/cpp/container/unordered_map/merge, should\n> > > do.\n> >\n> > At the moment the IPA does not report any metadata if I'm not\n> > mistaken, and this change breaks 80+ CTS tests as it overwrites the actual\n> > metadata with an empty list.\n>\n> Correct, so I'm fine with this patch, but we'll need to immediately\n> improve it on top :-)\n>\n> > I would also question that the pipeline handler should blindly merge\n> > the two as over-writes might happen, but I might be mistaken.\n>\n> In theory that's possible, but given that the pipeline handler and IPA\n> are supposed to be developed in sync, that's something the developer\n> should be able to control. They could either make sure not conflict\n> occurs, or implement a more elaborate merge method. I would expect the\n> former to be the common case, hence the proposal for\n> ControlList::merge().\n>\n> > Anyway, I'm fine having more CTS tests broken waiting for the IPA to\n> > actually set something there if that's the desired direction. I would\n> > prefer the other way around as this hinders CTS progresses but I would\n> > go with what the group prefers.\n>\n> No, we can merge this patch first, that's not an issue. The fact that\n> Niklas and I have pointed out issues doesn't mean they all have to be\n> fixed as a prerequisite, they can also go on top where it makes sense\n> :-)\n\nGreat, but I need one more tag to merge I think\n\n>\n> > > > If we wish to take this in the direction of this patch I think the IPA\n> > > > interface and IPA should also be updated to not report any metadata.\n> > > >\n> > > > > \t\tinfo->metadataProcessed = true;\n> > > > > \t\tif (frameInfos_.tryComplete(info))\n> > > > > \t\t\tpipe_->completeRequest(request);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 4ED17BD1F1\n\tfor ;\n\tMon, 22 Feb 2021 10:24:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5F7368A06;\n\tMon, 22 Feb 2021 11:24:15 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F4B5602F3\n\tfor ;\n\tMon, 22 Feb 2021 11:24:15 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 5B599C0015;\n\tMon, 22 Feb 2021 10:24:14 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 22 Feb 2021 11:24:41 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210222102441.xq27dckghjqqosnd@uno.localdomain>","References":"<20210219112257.53307-1-jacopo@jmondi.org>\n\t\n\t\n\t<20210222090108.c4ifaau5rzq5fd4n@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15282,"web_url":"https://patchwork.libcamera.org/comment/15282/","msgid":"","date":"2021-02-22T10:26:09","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Feb 22, 2021 at 11:24:41AM +0100, Jacopo Mondi wrote:\n> On Mon, Feb 22, 2021 at 11:18:51AM +0200, Laurent Pinchart wrote:\n> > On Mon, Feb 22, 2021 at 10:01:08AM +0100, Jacopo Mondi wrote:\n> > > On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:\n> > > > On Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:\n> > > > > On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:\n> > > > > > When a Request is completed upon receiving the IPA produced metadata,\n> > > > > > the metadata associated with the Request are over-written, deleting\n> > > > > > the information set, in example, at ImgU output buffer completion time.\n> > > > > >\n> > > > > > If any additional Request metadata should be registered by inspecting\n> > > > > > the IPA produced metadata it has to be done without deleting the already\n> > > > > > registered entries.\n> > > > > >\n> > > > > > Fix this by replacing the metadata over-write with a todo entry.\n> > > > > >\n> > > > > > This change fixes CTS which was broken due to missing metadata in\n> > > > > > the completed requests.\n> > > > > >\n> > > > > > Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > index 2aed826a892a..9e867ab2e98a 100644\n> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> > > > > > \t\tif (!info)\n> > > > > > \t\t\tbreak;\n> > > > > >\n> > > > > > +\t\t/*\n> > > > > > +\t\t * \\todo Parse the value of the controls returned by the IPA\n> > > > > > +\t\t * in action.controls to register additional request metadata.\n> > > > > > +\t\t */\n> > > > > > \t\tRequest *request = info->request;\n> > > > > > -\t\trequest->metadata() = action.controls;\n> > > > >\n> > > > > I understand the intent is to keep metadata set by the pipeline but this\n> > > > > change simply moves the problem. With this change any metadata reported\n> > > > > by the IPA is discarded. I thought the idea was that all metadata would\n> > > > > be generated by the IPA? If not I think we need to merge the two here.\n> > > >\n> > > > Merging the two seems like the right solution. The ControlList class\n> > > > doesn't support this at the moment, but it shouldn't be difficult to\n> > > > extend it. A merge() function, along the lines of\n> > > > https://en.cppreference.com/w/cpp/container/unordered_map/merge, should\n> > > > do.\n> > >\n> > > At the moment the IPA does not report any metadata if I'm not\n> > > mistaken, and this change breaks 80+ CTS tests as it overwrites the actual\n> > > metadata with an empty list.\n> >\n> > Correct, so I'm fine with this patch, but we'll need to immediately\n> > improve it on top :-)\n> >\n> > > I would also question that the pipeline handler should blindly merge\n> > > the two as over-writes might happen, but I might be mistaken.\n> >\n> > In theory that's possible, but given that the pipeline handler and IPA\n> > are supposed to be developed in sync, that's something the developer\n> > should be able to control. They could either make sure not conflict\n> > occurs, or implement a more elaborate merge method. I would expect the\n> > former to be the common case, hence the proposal for\n> > ControlList::merge().\n> >\n> > > Anyway, I'm fine having more CTS tests broken waiting for the IPA to\n> > > actually set something there if that's the desired direction. I would\n> > > prefer the other way around as this hinders CTS progresses but I would\n> > > go with what the group prefers.\n> >\n> > No, we can merge this patch first, that's not an issue. The fact that\n> > Niklas and I have pointed out issues doesn't mean they all have to be\n> > fixed as a prerequisite, they can also go on top where it makes sense\n> > :-)\n> \n> Great, but I need one more tag to merge I think\n\nReviewed-by: Laurent Pinchart \n\nWould you be able to implement merge() on top of this ?\n\n> > > > > If we wish to take this in the direction of this patch I think the IPA\n> > > > > interface and IPA should also be updated to not report any metadata.\n> > > > >\n> > > > > > \t\tinfo->metadataProcessed = true;\n> > > > > > \t\tif (frameInfos_.tryComplete(info))\n> > > > > > \t\t\tpipe_->completeRequest(request);","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 14F4DBD1F1\n\tfor ;\n\tMon, 22 Feb 2021 10:26:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A07B268A05;\n\tMon, 22 Feb 2021 11:26:38 +0100 (CET)","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 7417A602F3\n\tfor ;\n\tMon, 22 Feb 2021 11:26:36 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D460B344;\n\tMon, 22 Feb 2021 11:26:35 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dUYPA1c9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613989596;\n\tbh=X+k9vK+K+mX8t5dSNwnHao3eHkL5Xg54Fw9lUASSeAA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dUYPA1c95CvVsOO8I/81h4BxI97hdgGTg7gv4bvb52tzQarWHkhhJsDcQHUnHbSUZ\n\tkgX5Kxn2oi0WJ/Azb3n72Kh8nhAARXOCCJxOJhn1RxahASOc/M7hZjF6LxMKVvRxIn\n\tG9B6JuIb9BjA8c7rw+S/w4aGEur9oTcw6Nn524Fc=","Date":"Mon, 22 Feb 2021 12:26:09 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210219112257.53307-1-jacopo@jmondi.org>\n\t\n\t\n\t<20210222090108.c4ifaau5rzq5fd4n@uno.localdomain>\n\t\n\t<20210222102441.xq27dckghjqqosnd@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210222102441.xq27dckghjqqosnd@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15283,"web_url":"https://patchwork.libcamera.org/comment/15283/","msgid":"<20210222105412.p63vijhyeggrmfxq@uno.localdomain>","date":"2021-02-22T10:54:12","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Feb 22, 2021 at 12:26:09PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Feb 22, 2021 at 11:24:41AM +0100, Jacopo Mondi wrote:\n> > On Mon, Feb 22, 2021 at 11:18:51AM +0200, Laurent Pinchart wrote:\n> > > On Mon, Feb 22, 2021 at 10:01:08AM +0100, Jacopo Mondi wrote:\n> > > > On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:\n> > > > > On Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:\n> > > > > > On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:\n> > > > > > > When a Request is completed upon receiving the IPA produced metadata,\n> > > > > > > the metadata associated with the Request are over-written, deleting\n> > > > > > > the information set, in example, at ImgU output buffer completion time.\n> > > > > > >\n> > > > > > > If any additional Request metadata should be registered by inspecting\n> > > > > > > the IPA produced metadata it has to be done without deleting the already\n> > > > > > > registered entries.\n> > > > > > >\n> > > > > > > Fix this by replacing the metadata over-write with a todo entry.\n> > > > > > >\n> > > > > > > This change fixes CTS which was broken due to missing metadata in\n> > > > > > > the completed requests.\n> > > > > > >\n> > > > > > > Fixes: 9708f49fecf2 (\"libcamera: ipu3: Share parameter and statistic buffers with IPA\")\n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-\n> > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > index 2aed826a892a..9e867ab2e98a 100644\n> > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> > > > > > > \t\tif (!info)\n> > > > > > > \t\t\tbreak;\n> > > > > > >\n> > > > > > > +\t\t/*\n> > > > > > > +\t\t * \\todo Parse the value of the controls returned by the IPA\n> > > > > > > +\t\t * in action.controls to register additional request metadata.\n> > > > > > > +\t\t */\n> > > > > > > \t\tRequest *request = info->request;\n> > > > > > > -\t\trequest->metadata() = action.controls;\n> > > > > >\n> > > > > > I understand the intent is to keep metadata set by the pipeline but this\n> > > > > > change simply moves the problem. With this change any metadata reported\n> > > > > > by the IPA is discarded. I thought the idea was that all metadata would\n> > > > > > be generated by the IPA? If not I think we need to merge the two here.\n> > > > >\n> > > > > Merging the two seems like the right solution. The ControlList class\n> > > > > doesn't support this at the moment, but it shouldn't be difficult to\n> > > > > extend it. A merge() function, along the lines of\n> > > > > https://en.cppreference.com/w/cpp/container/unordered_map/merge, should\n> > > > > do.\n> > > >\n> > > > At the moment the IPA does not report any metadata if I'm not\n> > > > mistaken, and this change breaks 80+ CTS tests as it overwrites the actual\n> > > > metadata with an empty list.\n> > >\n> > > Correct, so I'm fine with this patch, but we'll need to immediately\n> > > improve it on top :-)\n> > >\n> > > > I would also question that the pipeline handler should blindly merge\n> > > > the two as over-writes might happen, but I might be mistaken.\n> > >\n> > > In theory that's possible, but given that the pipeline handler and IPA\n> > > are supposed to be developed in sync, that's something the developer\n> > > should be able to control. They could either make sure not conflict\n> > > occurs, or implement a more elaborate merge method. I would expect the\n> > > former to be the common case, hence the proposal for\n> > > ControlList::merge().\n> > >\n> > > > Anyway, I'm fine having more CTS tests broken waiting for the IPA to\n> > > > actually set something there if that's the desired direction. I would\n> > > > prefer the other way around as this hinders CTS progresses but I would\n> > > > go with what the group prefers.\n> > >\n> > > No, we can merge this patch first, that's not an issue. The fact that\n> > > Niklas and I have pointed out issues doesn't mean they all have to be\n> > > fixed as a prerequisite, they can also go on top where it makes sense\n> > > :-)\n> >\n> > Great, but I need one more tag to merge I think\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nThanks\n\n> Would you be able to implement merge() on top of this ?\n>\n\nI currently don't have a use case for this as for CTS only the\npipeline set metadata are required.\n\nI think this will most likely be in the development path of the IPA\ncomponent for IPU3.\n\n> > > > > > If we wish to take this in the direction of this patch I think the IPA\n> > > > > > interface and IPA should also be updated to not report any metadata.\n> > > > > >\n> > > > > > > \t\tinfo->metadataProcessed = true;\n> > > > > > > \t\tif (frameInfos_.tryComplete(info))\n> > > > > > > \t\t\tpipe_->completeRequest(request);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 F1A30BD1F6\n\tfor ;\n\tMon, 22 Feb 2021 10:53:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CBA0A68A14;\n\tMon, 22 Feb 2021 11:53:45 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48BEE68A0C\n\tfor ;\n\tMon, 22 Feb 2021 11:53:45 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 7FC854000C;\n\tMon, 22 Feb 2021 10:53:44 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 22 Feb 2021 11:54:12 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210222105412.p63vijhyeggrmfxq@uno.localdomain>","References":"<20210219112257.53307-1-jacopo@jmondi.org>\n\t\n\t\n\t<20210222090108.c4ifaau5rzq5fd4n@uno.localdomain>\n\t\n\t<20210222102441.xq27dckghjqqosnd@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write\n\tmetadata","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]