[{"id":15329,"web_url":"https://patchwork.libcamera.org/comment/15329/","msgid":"","date":"2021-02-28T15:49:54","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sebastian,\n\nThank you for the patch.\n\nOn Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n> This patch fixes a mismatch of image formats during the pipeline\n> creation of the RkISP1. The mismatch happens because the current code\n> does not check if the configured format exceeds the maximum viable\n> resolution of the ISP.\n> \n> Make sure to use a sensor format resolution that is smaller or equal to\n> the maximum allowed resolution for the RkISP1. The maximum resolution\n> is defined within the `rkisp1-common.h` file as:\n> define RKISP1_ISP_MAX_WIDTH 4032\n> define RKISP1_ISP_MAX_HEIGHT 3024\n> \n> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n> the configured resolution.\n> \n> This means that some camera sensors can never operate with their maximum\n> resolution, for example on my OV13850 camera sensor, there are two\n> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n> never be picked as it surpasses the maximum of the ISP.\n\nIt would have been nice if the ISP had an input crop rectangle :-S It\nwould have allowed us to crop the input image a little bit to 4032x2992\n(keeping the same aspect ratio) or 4032x3024 (4:3). Just\ndouble-checking, is there indeed no way to crop at the input, or could\nit be that it's not implemented in the driver ? I can't find the\n4032x3024 limit in the documentation I have access to.\n\n> Signed-off-by: Sebastian Fricke \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n> 1 file changed, 31 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 50eaa6a4..56a406c1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\treturn Invalid;\n> \t}\n> \n> -\t/* Select the sensor format. */\n> -\tSize maxSize;\n> +\t/* Get the ISP resolution limits */\n> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n\nRelated to 1/2, note that you don't necessarily need to store the ISP\npointer in RkISP1CameraData. You can access the pipeline handler here:\n\n\tPipelineHandlerRkISP1 *pipe =\n\t\tstatic_cast(data_->pipe_);\n\tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n\n> +\tif (ispFormats.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n> +\t\treturn Invalid;\n> +\t}\n\nMissing blank line.\n\n> +\t/*\n> +\t * The maximum resolution is identical for all media bus codes on\n> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n> +\t */\n> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n> +\n> +\t/*\n> +\t * Select the sensor format, use either the best fit to the configured\n> +\t * format or a specific sensor format, when getFormat would choose a\n> +\t * resolution that surpasses the ISP maximum.\n> +\t */\n> +\tSize maxSensorSize;\n> +\tfor (const Size &size : sensor->sizes()) {\n> +\t\tif (size.width > ispMaximum.width ||\n> +\t\t size.height > ispMaximum.height)\n> +\t\t\tcontinue;\n\nThis makes me think we need better comparison functions for the Size\nclass. Maybe a Size::contains() function ? It doesn't have to be part of\nthis series.\n\n> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n> +\t}\n\nMissing blank line here too.\n\nCould we move all the code above to\nPipelineHandlerRkISP1::createCamera() and store maxSensorSize in\nRkISP1CameraData, to avoid doing the calculation every time validate()\nis called ?\n\n\n> +\tSize maxConfigSize;\n> \tfor (const StreamConfiguration &cfg : config_)\n> -\t\tmaxSize = std::max(maxSize, cfg.size);\n> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n> +\n> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n> +\t maxConfigSize.width <= maxSensorSize.width)\n> +\t\tmaxSensorSize = maxConfigSize;\n> \n> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> -\t\t\t\t\t maxSize);\n> +\t\t\t\t\t maxSensorSize);\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\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 C588ABD808\n\tfor ;\n\tSun, 28 Feb 2021 15:50:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 277D968A6D;\n\tSun, 28 Feb 2021 16:50:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90E0668A45\n\tfor ;\n\tSun, 28 Feb 2021 16:50:23 +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 0919C80F;\n\tSun, 28 Feb 2021 16:50:22 +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=\"nQ/Zxn5R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614527423;\n\tbh=pE7ePrFZpddhO4gDsChE+YrZxAl/hPdppuYZ7xAu/ns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nQ/Zxn5R1KABYFFUhlixlfrr5oOo7tm6JGoFsVy9GoTp3oHccsA8f4OAiA68Ylh98\n\t+tKwAnYMHfbd8H5Q4EIG/NX1TisKY5pUvgEv4sYzw9f7tzIgJuxY28UnBoxTwcseqd\n\tj/4xxBINE9oIREF8VJXdlDxg/dYCWFU9nKml5JCI=","Date":"Sun, 28 Feb 2021 17:49:54 +0200","From":"Laurent Pinchart ","To":"Sebastian Fricke ","Message-ID":"","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210227180126.37591-3-sebastian.fricke@posteo.net>","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15344,"web_url":"https://patchwork.libcamera.org/comment/15344/","msgid":"","date":"2021-03-01T07:48:19","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"Hi\n\nOn 28.02.21 16:49, Laurent Pinchart wrote:\n> Hi Sebastian,\n> \n> Thank you for the patch.\n> \n> On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n>> This patch fixes a mismatch of image formats during the pipeline\n>> creation of the RkISP1. The mismatch happens because the current code\n>> does not check if the configured format exceeds the maximum viable\n>> resolution of the ISP.\n>>\n>> Make sure to use a sensor format resolution that is smaller or equal to\n>> the maximum allowed resolution for the RkISP1. The maximum resolution\n>> is defined within the `rkisp1-common.h` file as:\n>> define RKISP1_ISP_MAX_WIDTH 4032\n>> define RKISP1_ISP_MAX_HEIGHT 3024\n>>\n>> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n>> the configured resolution.\n>>\n>> This means that some camera sensors can never operate with their maximum\n>> resolution, for example on my OV13850 camera sensor, there are two\n>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n>> never be picked as it surpasses the maximum of the ISP.\n> \n> It would have been nice if the ISP had an input crop rectangle :-S It\n> would have allowed us to crop the input image a little bit to 4032x2992\n> (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n> double-checking, is there indeed no way to crop at the input, or could\n> it be that it's not implemented in the driver ? I can't find the\n> 4032x3024 limit in the documentation I have access to.\n\nThe isp does support cropping on the sink pad.\nBut currently the function v4l2_subdev_link_validate_default compares\nthe dimensions defined in v4l2_subdev_format which are not the crop\ndimensions but the ones set by set_fmt. Is that wrong?\n\n> \n>> Signed-off-by: Sebastian Fricke \n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n>> 1 file changed, 31 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index 50eaa6a4..56a406c1 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \t\treturn Invalid;\n>> \t}\n>> \n>> -\t/* Select the sensor format. */\n>> -\tSize maxSize;\n>> +\t/* Get the ISP resolution limits */\n>> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n> \n> Related to 1/2, note that you don't necessarily need to store the ISP\n> pointer in RkISP1CameraData. You can access the pipeline handler here:\n> \n> \tPipelineHandlerRkISP1 *pipe =\n> \t\tstatic_cast(data_->pipe_);\n> \tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n> \n>> +\tif (ispFormats.empty()) {\n>> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n>> +\t\treturn Invalid;\n>> +\t}\n> \n> Missing blank line.\n> \n>> +\t/*\n>> +\t * The maximum resolution is identical for all media bus codes on\n>> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n>> +\t */\n>> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n>> +\n>> +\t/*\n>> +\t * Select the sensor format, use either the best fit to the configured\n>> +\t * format or a specific sensor format, when getFormat would choose a\n>> +\t * resolution that surpasses the ISP maximum.\n>> +\t */\n>> +\tSize maxSensorSize;\n>> +\tfor (const Size &size : sensor->sizes()) {\n>> +\t\tif (size.width > ispMaximum.width ||\n>> +\t\t size.height > ispMaximum.height)\n>> +\t\t\tcontinue;\n> \n> This makes me think we need better comparison functions for the Size\n> class. Maybe a Size::contains() function ? It doesn't have to be part of\n> this series.\n> \n>> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n>> +\t}\n> \n> Missing blank line here too.\n> \n> Could we move all the code above to\n> PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n> RkISP1CameraData, to avoid doing the calculation every time validate()\n> is called ?\n> \n> \n>> +\tSize maxConfigSize;\n>> \tfor (const StreamConfiguration &cfg : config_)\n>> -\t\tmaxSize = std::max(maxSize, cfg.size);\n>> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n>> +\n>> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n>> +\t maxConfigSize.width <= maxSensorSize.width)\n>> +\t\tmaxSensorSize = maxConfigSize;\n>> \n>> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n\nI wonder if it won't be an easier solution to add another optional parameter\nto the getFormat method of the sensor that limits the size that the sensor\nshould return. This might benefit other pipline-handlers as well where\na sensor is connected to an entity that has a maximal size it can accept.\nIn rkisp1 all the above code could be replaced by just adding\nSize(4032,3024) as another parameter to getFormat.\n\nThanks,\nDafna\n\n>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n>> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n>> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n>> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n>> -\t\t\t\t\t maxSize);\n>> +\t\t\t\t\t maxSensorSize);\n>> \tif (sensorFormat_.size.isNull())\n>> \t\tsensorFormat_.size = sensor->resolution();\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 B8BE4BD1F1\n\tfor ;\n\tMon, 1 Mar 2021 07:48:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55D1B68A7E;\n\tMon, 1 Mar 2021 08:48:24 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93151602EC\n\tfor ;\n\tMon, 1 Mar 2021 08:48:23 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: dafna) with ESMTPSA id 1DE6E1F45004"],"To":"Laurent Pinchart ,\n\tSebastian Fricke ","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t","From":"Dafna Hirschfeld ","Message-ID":"","Date":"Mon, 1 Mar 2021 08:48:19 +0100","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":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15389,"web_url":"https://patchwork.libcamera.org/comment/15389/","msgid":"","date":"2021-03-02T02:39:43","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nOn Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:\n> On 28.02.21 16:49, Laurent Pinchart wrote:\n> > On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n> >> This patch fixes a mismatch of image formats during the pipeline\n> >> creation of the RkISP1. The mismatch happens because the current code\n> >> does not check if the configured format exceeds the maximum viable\n> >> resolution of the ISP.\n> >>\n> >> Make sure to use a sensor format resolution that is smaller or equal to\n> >> the maximum allowed resolution for the RkISP1. The maximum resolution\n> >> is defined within the `rkisp1-common.h` file as:\n> >> define RKISP1_ISP_MAX_WIDTH 4032\n> >> define RKISP1_ISP_MAX_HEIGHT 3024\n> >>\n> >> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n> >> the configured resolution.\n> >>\n> >> This means that some camera sensors can never operate with their maximum\n> >> resolution, for example on my OV13850 camera sensor, there are two\n> >> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n> >> never be picked as it surpasses the maximum of the ISP.\n> > \n> > It would have been nice if the ISP had an input crop rectangle :-S It\n> > would have allowed us to crop the input image a little bit to 4032x2992\n> > (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n> > double-checking, is there indeed no way to crop at the input, or could\n> > it be that it's not implemented in the driver ? I can't find the\n> > 4032x3024 limit in the documentation I have access to.\n> \n> The isp does support cropping on the sink pad.\n> But currently the function v4l2_subdev_link_validate_default compares\n> the dimensions defined in v4l2_subdev_format which are not the crop\n> dimensions but the ones set by set_fmt. Is that wrong?\n\nI think so. If the ISP supports a larger size than 4032x3024 before\ncropping, it should accept that on its sink pad, with the sink crop\nrectangle being adjusted to never be larger than 4032x3024 (for instance\nwhen userspace sets the format on the sink pad, the crop rectangle could\nbe automatically set to the same size, clamped to 4032x3024 and\ncentered). Userspace can then adjust the crop rectangle further if\nnecessary.\n\n> >> Signed-off-by: Sebastian Fricke \n> >> ---\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n> >> 1 file changed, 31 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index 50eaa6a4..56a406c1 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >> \t\treturn Invalid;\n> >> \t}\n> >> \n> >> -\t/* Select the sensor format. */\n> >> -\tSize maxSize;\n> >> +\t/* Get the ISP resolution limits */\n> >> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n> > \n> > Related to 1/2, note that you don't necessarily need to store the ISP\n> > pointer in RkISP1CameraData. You can access the pipeline handler here:\n> > \n> > \tPipelineHandlerRkISP1 *pipe =\n> > \t\tstatic_cast(data_->pipe_);\n> > \tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n> > \n> >> +\tif (ispFormats.empty()) {\n> >> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n> >> +\t\treturn Invalid;\n> >> +\t}\n> > \n> > Missing blank line.\n> > \n> >> +\t/*\n> >> +\t * The maximum resolution is identical for all media bus codes on\n> >> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n> >> +\t */\n> >> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n> >> +\n> >> +\t/*\n> >> +\t * Select the sensor format, use either the best fit to the configured\n> >> +\t * format or a specific sensor format, when getFormat would choose a\n> >> +\t * resolution that surpasses the ISP maximum.\n> >> +\t */\n> >> +\tSize maxSensorSize;\n> >> +\tfor (const Size &size : sensor->sizes()) {\n> >> +\t\tif (size.width > ispMaximum.width ||\n> >> +\t\t size.height > ispMaximum.height)\n> >> +\t\t\tcontinue;\n> > \n> > This makes me think we need better comparison functions for the Size\n> > class. Maybe a Size::contains() function ? It doesn't have to be part of\n> > this series.\n> > \n> >> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n> >> +\t}\n> > \n> > Missing blank line here too.\n> > \n> > Could we move all the code above to\n> > PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n> > RkISP1CameraData, to avoid doing the calculation every time validate()\n> > is called ?\n> > \n> > \n> >> +\tSize maxConfigSize;\n> >> \tfor (const StreamConfiguration &cfg : config_)\n> >> -\t\tmaxSize = std::max(maxSize, cfg.size);\n> >> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n> >> +\n> >> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n> >> +\t maxConfigSize.width <= maxSensorSize.width)\n> >> +\t\tmaxSensorSize = maxConfigSize;\n> >> \n> >> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> \n> I wonder if it won't be an easier solution to add another optional parameter\n> to the getFormat method of the sensor that limits the size that the sensor\n> should return. This might benefit other pipline-handlers as well where\n> a sensor is connected to an entity that has a maximal size it can accept.\n> In rkisp1 all the above code could be replaced by just adding\n> Size(4032,3024) as another parameter to getFormat.\n>\n> >> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> >> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> >> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> >> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> >> -\t\t\t\t\t maxSize);\n> >> +\t\t\t\t\t maxSensorSize);\n> >> \tif (sensorFormat_.size.isNull())\n> >> \t\tsensorFormat_.size = sensor->resolution();\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 6D7C5BD1F1\n\tfor ;\n\tTue, 2 Mar 2021 02:40:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F121968A8D;\n\tTue, 2 Mar 2021 03:40:14 +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 6D18160522\n\tfor ;\n\tTue, 2 Mar 2021 03:40:13 +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 CE8D045D;\n\tTue, 2 Mar 2021 03:40:12 +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=\"vUkKYcRa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614652813;\n\tbh=2ppitFx0MIoSYs+FTLwJaA//B/lWHGfpXznuqFYExSI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vUkKYcRa8ScxnU0LnY2qS8hAG0AFN1Gz3OUlB9Aq0GCycrV+mBgCIG7F1o3V4HHxW\n\t2hANzo/FiIqq3vmqTFRaen4wUFXkSd2385pO4qcTQ5gOCCIlBeM8eU3dbKv3U8dobJ\n\t/ddkAfIRQRHodw51fL6dFc9zhFstj4TJ7+RTy4yU=","Date":"Tue, 2 Mar 2021 04:39:43 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15397,"web_url":"https://patchwork.libcamera.org/comment/15397/","msgid":"<6274ed99-d62a-a995-3b79-4d0f76c533d6@collabora.com>","date":"2021-03-02T08:16:04","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"On 02.03.21 03:39, Laurent Pinchart wrote:\n> Hi Dafna,\n> \n> On Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:\n>> On 28.02.21 16:49, Laurent Pinchart wrote:\n>>> On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n>>>> This patch fixes a mismatch of image formats during the pipeline\n>>>> creation of the RkISP1. The mismatch happens because the current code\n>>>> does not check if the configured format exceeds the maximum viable\n>>>> resolution of the ISP.\n>>>>\n>>>> Make sure to use a sensor format resolution that is smaller or equal to\n>>>> the maximum allowed resolution for the RkISP1. The maximum resolution\n>>>> is defined within the `rkisp1-common.h` file as:\n>>>> define RKISP1_ISP_MAX_WIDTH 4032\n>>>> define RKISP1_ISP_MAX_HEIGHT 3024\n>>>>\n>>>> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n>>>> the configured resolution.\n>>>>\n>>>> This means that some camera sensors can never operate with their maximum\n>>>> resolution, for example on my OV13850 camera sensor, there are two\n>>>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n>>>> never be picked as it surpasses the maximum of the ISP.\n>>>\n>>> It would have been nice if the ISP had an input crop rectangle :-S It\n>>> would have allowed us to crop the input image a little bit to 4032x2992\n>>> (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n>>> double-checking, is there indeed no way to crop at the input, or could\n>>> it be that it's not implemented in the driver ? I can't find the\n>>> 4032x3024 limit in the documentation I have access to.\n>>\n>> The isp does support cropping on the sink pad.\n>> But currently the function v4l2_subdev_link_validate_default compares\n>> the dimensions defined in v4l2_subdev_format which are not the crop\n>> dimensions but the ones set by set_fmt. Is that wrong?\n> \n> I think so. If the ISP supports a larger size than 4032x3024 before\n> cropping, it should accept that on its sink pad, with the sink crop\n> rectangle being adjusted to never be larger than 4032x3024 (for instance\n> when userspace sets the format on the sink pad, the crop rectangle could\n> be automatically set to the same size, clamped to 4032x3024 and\n> centered). Userspace can then adjust the crop rectangle further if\n> necessary.\n\nIn rkisp1-isp.c, there is diagram of the cropping regions.\nIt says that the 4032x3024 limit is 'for black level'.\nDoes that means that some sensors might send frames with black pixels in\nthe edges that need to be cropped?\n\nThe TRM says \"Maximum input resolution of 4416x3312 pixels\"\nThe datasheet then shows that the default values are 4096x3072 for both the\ninput resolution (ISP_ACQ) and for the corp in the sink (ISP_OUT).\n\nSo from the docs at least it sound possible to do as you suggested,\nlimit the input to 4416x3312 and then always set a crop with maximum\nvalue 4032x3024\n\nThanks,\nDafna\n\n\n\n\n> \n>>>> Signed-off-by: Sebastian Fricke \n>>>> ---\n>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n>>>> 1 file changed, 31 insertions(+), 4 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> index 50eaa6a4..56a406c1 100644\n>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>> \t\treturn Invalid;\n>>>> \t}\n>>>> \n>>>> -\t/* Select the sensor format. */\n>>>> -\tSize maxSize;\n>>>> +\t/* Get the ISP resolution limits */\n>>>> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n>>>\n>>> Related to 1/2, note that you don't necessarily need to store the ISP\n>>> pointer in RkISP1CameraData. You can access the pipeline handler here:\n>>>\n>>> \tPipelineHandlerRkISP1 *pipe =\n>>> \t\tstatic_cast(data_->pipe_);\n>>> \tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n>>>\n>>>> +\tif (ispFormats.empty()) {\n>>>> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n>>>> +\t\treturn Invalid;\n>>>> +\t}\n>>>\n>>> Missing blank line.\n>>>\n>>>> +\t/*\n>>>> +\t * The maximum resolution is identical for all media bus codes on\n>>>> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n>>>> +\t */\n>>>> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n>>>> +\n>>>> +\t/*\n>>>> +\t * Select the sensor format, use either the best fit to the configured\n>>>> +\t * format or a specific sensor format, when getFormat would choose a\n>>>> +\t * resolution that surpasses the ISP maximum.\n>>>> +\t */\n>>>> +\tSize maxSensorSize;\n>>>> +\tfor (const Size &size : sensor->sizes()) {\n>>>> +\t\tif (size.width > ispMaximum.width ||\n>>>> +\t\t size.height > ispMaximum.height)\n>>>> +\t\t\tcontinue;\n>>>\n>>> This makes me think we need better comparison functions for the Size\n>>> class. Maybe a Size::contains() function ? It doesn't have to be part of\n>>> this series.\n>>>\n>>>> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n>>>> +\t}\n>>>\n>>> Missing blank line here too.\n>>>\n>>> Could we move all the code above to\n>>> PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n>>> RkISP1CameraData, to avoid doing the calculation every time validate()\n>>> is called ?\n>>>\n>>>\n>>>> +\tSize maxConfigSize;\n>>>> \tfor (const StreamConfiguration &cfg : config_)\n>>>> -\t\tmaxSize = std::max(maxSize, cfg.size);\n>>>> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n>>>> +\n>>>> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n>>>> +\t maxConfigSize.width <= maxSensorSize.width)\n>>>> +\t\tmaxSensorSize = maxConfigSize;\n>>>> \n>>>> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n>>\n>> I wonder if it won't be an easier solution to add another optional parameter\n>> to the getFormat method of the sensor that limits the size that the sensor\n>> should return. This might benefit other pipline-handlers as well where\n>> a sensor is connected to an entity that has a maximal size it can accept.\n>> In rkisp1 all the above code could be replaced by just adding\n>> Size(4032,3024) as another parameter to getFormat.\n>>\n>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n>>>> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n>>>> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n>>>> -\t\t\t\t\t maxSize);\n>>>> +\t\t\t\t\t maxSensorSize);\n>>>> \tif (sensorFormat_.size.isNull())\n>>>> \t\tsensorFormat_.size = sensor->resolution();\n>>>> \n>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6CEFBBD1F1\n\tfor ;\n\tTue, 2 Mar 2021 08:16:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4D7468A92;\n\tTue, 2 Mar 2021 09:16:08 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37FCB68A7F\n\tfor ;\n\tTue, 2 Mar 2021 09:16:08 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: dafna) with ESMTPSA id 7DA051F452D2"],"To":"Laurent Pinchart ","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t\n\t","From":"Dafna Hirschfeld ","Message-ID":"<6274ed99-d62a-a995-3b79-4d0f76c533d6@collabora.com>","Date":"Tue, 2 Mar 2021 09:16:04 +0100","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":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15412,"web_url":"https://patchwork.libcamera.org/comment/15412/","msgid":"<1818344.taCxCBeP46@phil>","date":"2021-03-02T11:07:28","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":83,"url":"https://patchwork.libcamera.org/api/people/83/","name":"Heiko Stübner","email":"heiko@sntech.de"},"content":"Am Samstag, 27. Februar 2021, 19:01:26 CET schrieb Sebastian Fricke:\n> This patch fixes a mismatch of image formats during the pipeline\n> creation of the RkISP1. The mismatch happens because the current code\n> does not check if the configured format exceeds the maximum viable\n> resolution of the ISP.\n> \n> Make sure to use a sensor format resolution that is smaller or equal to\n> the maximum allowed resolution for the RkISP1. The maximum resolution\n> is defined within the `rkisp1-common.h` file as:\n> define RKISP1_ISP_MAX_WIDTH 4032\n> define RKISP1_ISP_MAX_HEIGHT 3024\n\nhmm, somehow these sizes look interesting, but this patch triggered me\ninto looking this up :-)\n\nI.e. in the vendor-kernel Rockchip defines [0] a number of different max-sizes\nand none seem to match these 4032.\n\n#define CIF_ISP_INPUT_W_MAX\t\t4416\n#define CIF_ISP_INPUT_H_MAX\t\t3312\n#define CIF_ISP_INPUT_W_MAX_V12\t3264\n#define CIF_ISP_INPUT_H_MAX_V12\t2448\n#define CIF_ISP_INPUT_W_MAX_V13\t1920\n#define CIF_ISP_INPUT_H_MAX_V13\t1080\n\nFunnily enough my (old) rk3399 datasheet specifies 3264x2448 as max\nISP input size, but I guess the vendor-code will be \"more\" correct.\n\nAt least I understand now that I need to also handle different sizes when\nupdating my V12 patches for the kernel-side.\n\n\nHeiko\n\n[0] https://github.com/rockchip-linux/kernel/commit/40ce742d635eb8f821009b20f09668f4a1261e47\n\n> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n> the configured resolution.\n> \n> This means that some camera sensors can never operate with their maximum\n> resolution, for example on my OV13850 camera sensor, there are two\n> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n> never be picked as it surpasses the maximum of the ISP.\n>\n> Signed-off-by: Sebastian Fricke \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n> 1 file changed, 31 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 50eaa6a4..56a406c1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\treturn Invalid;\n> \t}\n> \n> -\t/* Select the sensor format. */\n> -\tSize maxSize;\n> +\t/* Get the ISP resolution limits */\n> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n> +\tif (ispFormats.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n> +\t\treturn Invalid;\n> +\t}\n> +\t/*\n> +\t * The maximum resolution is identical for all media bus codes on\n> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n> +\t */\n> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n> +\n> +\t/*\n> +\t * Select the sensor format, use either the best fit to the configured\n> +\t * format or a specific sensor format, when getFormat would choose a\n> +\t * resolution that surpasses the ISP maximum.\n> +\t */\n> +\tSize maxSensorSize;\n> +\tfor (const Size &size : sensor->sizes()) {\n> +\t\tif (size.width > ispMaximum.width ||\n> +\t\t size.height > ispMaximum.height)\n> +\t\t\tcontinue;\n> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n> +\t}\n> +\tSize maxConfigSize;\n> \tfor (const StreamConfiguration &cfg : config_)\n> -\t\tmaxSize = std::max(maxSize, cfg.size);\n> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n> +\n> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n> +\t maxConfigSize.width <= maxSensorSize.width)\n> +\t\tmaxSensorSize = maxConfigSize;\n> \n> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> -\t\t\t\t\t maxSize);\n> +\t\t\t\t\t maxSensorSize);\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\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 D1B5CBD808\n\tfor ;\n\tTue, 2 Mar 2021 11:07:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9F9068A97;\n\tTue, 2 Mar 2021 12:07:31 +0100 (CET)","from gloria.sntech.de (gloria.sntech.de [185.11.138.130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7701468A7E\n\tfor ;\n\tTue, 2 Mar 2021 12:07:30 +0100 (CET)","from p578c1707.dip0.t-ipconnect.de ([87.140.23.7]\n\thelo=phil.localnet) by gloria.sntech.de with esmtpsa\n\t(TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)\n\t(Exim 4.92) (envelope-from )\n\tid 1lH2sD-0000Lw-Hv; Tue, 02 Mar 2021 12:07:29 +0100"],"From":"heiko@sntech.de","To":"libcamera-devel@lists.libcamera.org","Date":"Tue, 02 Mar 2021 12:07:28 +0100","Message-ID":"<1818344.taCxCBeP46@phil>","In-Reply-To":"<20210227180126.37591-3-sebastian.fricke@posteo.net>","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15633,"web_url":"https://patchwork.libcamera.org/comment/15633/","msgid":"","date":"2021-03-12T21:31:03","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nOn Tue, Mar 02, 2021 at 09:16:04AM +0100, Dafna Hirschfeld wrote:\n> On 02.03.21 03:39, Laurent Pinchart wrote:\n> > On Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:\n> >> On 28.02.21 16:49, Laurent Pinchart wrote:\n> >>> On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n> >>>> This patch fixes a mismatch of image formats during the pipeline\n> >>>> creation of the RkISP1. The mismatch happens because the current code\n> >>>> does not check if the configured format exceeds the maximum viable\n> >>>> resolution of the ISP.\n> >>>>\n> >>>> Make sure to use a sensor format resolution that is smaller or equal to\n> >>>> the maximum allowed resolution for the RkISP1. The maximum resolution\n> >>>> is defined within the `rkisp1-common.h` file as:\n> >>>> define RKISP1_ISP_MAX_WIDTH 4032\n> >>>> define RKISP1_ISP_MAX_HEIGHT 3024\n> >>>>\n> >>>> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n> >>>> the configured resolution.\n> >>>>\n> >>>> This means that some camera sensors can never operate with their maximum\n> >>>> resolution, for example on my OV13850 camera sensor, there are two\n> >>>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n> >>>> never be picked as it surpasses the maximum of the ISP.\n> >>>\n> >>> It would have been nice if the ISP had an input crop rectangle :-S It\n> >>> would have allowed us to crop the input image a little bit to 4032x2992\n> >>> (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n> >>> double-checking, is there indeed no way to crop at the input, or could\n> >>> it be that it's not implemented in the driver ? I can't find the\n> >>> 4032x3024 limit in the documentation I have access to.\n> >>\n> >> The isp does support cropping on the sink pad.\n> >> But currently the function v4l2_subdev_link_validate_default compares\n> >> the dimensions defined in v4l2_subdev_format which are not the crop\n> >> dimensions but the ones set by set_fmt. Is that wrong?\n> > \n> > I think so. If the ISP supports a larger size than 4032x3024 before\n> > cropping, it should accept that on its sink pad, with the sink crop\n> > rectangle being adjusted to never be larger than 4032x3024 (for instance\n> > when userspace sets the format on the sink pad, the crop rectangle could\n> > be automatically set to the same size, clamped to 4032x3024 and\n> > centered). Userspace can then adjust the crop rectangle further if\n> > necessary.\n> \n> In rkisp1-isp.c, there is diagram of the cropping regions.\n> It says that the 4032x3024 limit is 'for black level'.\n> Does that means that some sensors might send frames with black pixels in\n> the edges that need to be cropped?\n\nIn this context, I believe that black level refers to black level\ncorrection (BLC in short), which is the process of subtracting a fixed\nvalue from all pixels to account for leakage currents and other\nparasitic effects in the sensor that makes fully black pixels have a\nnon-zero value. Sensors typically have a set of lines before the image\nthat is physically covered, and those lines can then be transmitted over\nCSI-2. The average black level will then be computed on the SoC side\n(either using the CPU, or in the ISP if it supports doing so), and\nconfigured in the ISP to subtract it from all pixels. The black lines\nare cropped out of what the ISP processes further down the pipeline.\n\n> The TRM says \"Maximum input resolution of 4416x3312 pixels\"\n> The datasheet then shows that the default values are 4096x3072 for both the\n> input resolution (ISP_ACQ) and for the corp in the sink (ISP_OUT).\n> \n> So from the docs at least it sound possible to do as you suggested,\n> limit the input to 4416x3312 and then always set a crop with maximum\n> value 4032x3024\n\nSounds like it's worth a try at least :-)\n\n> >>>> Signed-off-by: Sebastian Fricke \n> >>>> ---\n> >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n> >>>> 1 file changed, 31 insertions(+), 4 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> index 50eaa6a4..56a406c1 100644\n> >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>>> \t\treturn Invalid;\n> >>>> \t}\n> >>>> \n> >>>> -\t/* Select the sensor format. */\n> >>>> -\tSize maxSize;\n> >>>> +\t/* Get the ISP resolution limits */\n> >>>> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n> >>>\n> >>> Related to 1/2, note that you don't necessarily need to store the ISP\n> >>> pointer in RkISP1CameraData. You can access the pipeline handler here:\n> >>>\n> >>> \tPipelineHandlerRkISP1 *pipe =\n> >>> \t\tstatic_cast(data_->pipe_);\n> >>> \tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n> >>>\n> >>>> +\tif (ispFormats.empty()) {\n> >>>> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n> >>>> +\t\treturn Invalid;\n> >>>> +\t}\n> >>>\n> >>> Missing blank line.\n> >>>\n> >>>> +\t/*\n> >>>> +\t * The maximum resolution is identical for all media bus codes on\n> >>>> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n> >>>> +\t */\n> >>>> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n> >>>> +\n> >>>> +\t/*\n> >>>> +\t * Select the sensor format, use either the best fit to the configured\n> >>>> +\t * format or a specific sensor format, when getFormat would choose a\n> >>>> +\t * resolution that surpasses the ISP maximum.\n> >>>> +\t */\n> >>>> +\tSize maxSensorSize;\n> >>>> +\tfor (const Size &size : sensor->sizes()) {\n> >>>> +\t\tif (size.width > ispMaximum.width ||\n> >>>> +\t\t size.height > ispMaximum.height)\n> >>>> +\t\t\tcontinue;\n> >>>\n> >>> This makes me think we need better comparison functions for the Size\n> >>> class. Maybe a Size::contains() function ? It doesn't have to be part of\n> >>> this series.\n> >>>\n> >>>> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n> >>>> +\t}\n> >>>\n> >>> Missing blank line here too.\n> >>>\n> >>> Could we move all the code above to\n> >>> PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n> >>> RkISP1CameraData, to avoid doing the calculation every time validate()\n> >>> is called ?\n> >>>\n> >>>\n> >>>> +\tSize maxConfigSize;\n> >>>> \tfor (const StreamConfiguration &cfg : config_)\n> >>>> -\t\tmaxSize = std::max(maxSize, cfg.size);\n> >>>> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n> >>>> +\n> >>>> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n> >>>> +\t maxConfigSize.width <= maxSensorSize.width)\n> >>>> +\t\tmaxSensorSize = maxConfigSize;\n> >>>> \n> >>>> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> >>\n> >> I wonder if it won't be an easier solution to add another optional parameter\n> >> to the getFormat method of the sensor that limits the size that the sensor\n> >> should return. This might benefit other pipline-handlers as well where\n> >> a sensor is connected to an entity that has a maximal size it can accept.\n> >> In rkisp1 all the above code could be replaced by just adding\n> >> Size(4032,3024) as another parameter to getFormat.\n> >>\n> >>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> >>>> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> >>>> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> >>>> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> >>>> -\t\t\t\t\t maxSize);\n> >>>> +\t\t\t\t\t maxSensorSize);\n> >>>> \tif (sensorFormat_.size.isNull())\n> >>>> \t\tsensorFormat_.size = sensor->resolution();\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 1A75DBD1F1\n\tfor ;\n\tFri, 12 Mar 2021 21:31:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 583B268C68;\n\tFri, 12 Mar 2021 22:31:40 +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 C456568AA1\n\tfor ;\n\tFri, 12 Mar 2021 22:31:38 +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 2E14488F;\n\tFri, 12 Mar 2021 22:31:38 +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=\"bDxrnFZr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615584698;\n\tbh=ziJ6qPBcDTgw96IkLKGtdgHwZKweI1ZJaVdJ0+m3G8U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bDxrnFZroqIs8xjnQLedCM/EWOgTv7KEnLOfuRP8hq50B/Cmo3cUiqoDPReP829iW\n\tlnge7Je5+Uiog1Pt2C/9Pnaj6NU9KOOMYZXx8v/yg2JmkZ3Ww3E8DBJeciTL5ohjJS\n\tJZzItD92UXVroAyW2trlGrtubc2Kti+QuADSGhuY=","Date":"Fri, 12 Mar 2021 23:31:03 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t\n\t\n\t<6274ed99-d62a-a995-3b79-4d0f76c533d6@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<6274ed99-d62a-a995-3b79-4d0f76c533d6@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15709,"web_url":"https://patchwork.libcamera.org/comment/15709/","msgid":"<8f1b35b1-feb8-7e04-095a-d70c8db315d3@collabora.com>","date":"2021-03-15T16:52:14","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"On 12.03.21 22:31, Laurent Pinchart wrote:\n> Hi Dafna,\n> \n> On Tue, Mar 02, 2021 at 09:16:04AM +0100, Dafna Hirschfeld wrote:\n>> On 02.03.21 03:39, Laurent Pinchart wrote:\n>>> On Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:\n>>>> On 28.02.21 16:49, Laurent Pinchart wrote:\n>>>>> On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n>>>>>> This patch fixes a mismatch of image formats during the pipeline\n>>>>>> creation of the RkISP1. The mismatch happens because the current code\n>>>>>> does not check if the configured format exceeds the maximum viable\n>>>>>> resolution of the ISP.\n>>>>>>\n>>>>>> Make sure to use a sensor format resolution that is smaller or equal to\n>>>>>> the maximum allowed resolution for the RkISP1. The maximum resolution\n>>>>>> is defined within the `rkisp1-common.h` file as:\n>>>>>> define RKISP1_ISP_MAX_WIDTH 4032\n>>>>>> define RKISP1_ISP_MAX_HEIGHT 3024\n>>>>>>\n>>>>>> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n>>>>>> the configured resolution.\n>>>>>>\n>>>>>> This means that some camera sensors can never operate with their maximum\n>>>>>> resolution, for example on my OV13850 camera sensor, there are two\n>>>>>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n>>>>>> never be picked as it surpasses the maximum of the ISP.\n>>>>>\n>>>>> It would have been nice if the ISP had an input crop rectangle :-S It\n>>>>> would have allowed us to crop the input image a little bit to 4032x2992\n>>>>> (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n>>>>> double-checking, is there indeed no way to crop at the input, or could\n>>>>> it be that it's not implemented in the driver ? I can't find the\n>>>>> 4032x3024 limit in the documentation I have access to.\n>>>>\n>>>> The isp does support cropping on the sink pad.\n>>>> But currently the function v4l2_subdev_link_validate_default compares\n>>>> the dimensions defined in v4l2_subdev_format which are not the crop\n>>>> dimensions but the ones set by set_fmt. Is that wrong?\n>>>\n>>> I think so. If the ISP supports a larger size than 4032x3024 before\n>>> cropping, it should accept that on its sink pad, with the sink crop\n>>> rectangle being adjusted to never be larger than 4032x3024 (for instance\n>>> when userspace sets the format on the sink pad, the crop rectangle could\n>>> be automatically set to the same size, clamped to 4032x3024 and\n>>> centered). Userspace can then adjust the crop rectangle further if\n>>> necessary.\n>>\n>> In rkisp1-isp.c, there is diagram of the cropping regions.\n>> It says that the 4032x3024 limit is 'for black level'.\n>> Does that means that some sensors might send frames with black pixels in\n>> the edges that need to be cropped?\n> \n> In this context, I believe that black level refers to black level\n> correction (BLC in short), which is the process of subtracting a fixed\n> value from all pixels to account for leakage currents and other\n> parasitic effects in the sensor that makes fully black pixels have a\n> non-zero value. Sensors typically have a set of lines before the image\n> that is physically covered, and those lines can then be transmitted over\n> CSI-2. The average black level will then be computed on the SoC side\n> (either using the CPU, or in the ISP if it supports doing so), and\n> configured in the ISP to subtract it from all pixels. The black lines\n> are cropped out of what the ISP processes further down the pipeline.\n\nThe number of black/invalid lines depends on the sensor right?\nSo userspace should adjust the cropping according to the sensor.\nIf so then why would we want to always clamp to 4032x3024 ?\nOr should it just be the initial value and userspace can then increase\nthe crop size?\n\n> \n>> The TRM says \"Maximum input resolution of 4416x3312 pixels\"\n>> The datasheet then shows that the default values are 4096x3072 for both the\n>> input resolution (ISP_ACQ) and for the corp in the sink (ISP_OUT).\n>>\n>> So from the docs at least it sound possible to do as you suggested,\n>> limit the input to 4416x3312 and then always set a crop with maximum\n>> value 4032x3024\n> \n> Sounds like it's worth a try at least :-)\n> \n>>>>>> Signed-off-by: Sebastian Fricke \n>>>>>> ---\n>>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n>>>>>> 1 file changed, 31 insertions(+), 4 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>>> index 50eaa6a4..56a406c1 100644\n>>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>>> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>>>> \t\treturn Invalid;\n>>>>>> \t}\n>>>>>> \n>>>>>> -\t/* Select the sensor format. */\n>>>>>> -\tSize maxSize;\n>>>>>> +\t/* Get the ISP resolution limits */\n>>>>>> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n>>>>>\n>>>>> Related to 1/2, note that you don't necessarily need to store the ISP\n>>>>> pointer in RkISP1CameraData. You can access the pipeline handler here:\n>>>>>\n>>>>> \tPipelineHandlerRkISP1 *pipe =\n>>>>> \t\tstatic_cast(data_->pipe_);\n>>>>> \tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n>>>>>\n>>>>>> +\tif (ispFormats.empty()) {\n>>>>>> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n>>>>>> +\t\treturn Invalid;\n>>>>>> +\t}\n>>>>>\n>>>>> Missing blank line.\n>>>>>\n>>>>>> +\t/*\n>>>>>> +\t * The maximum resolution is identical for all media bus codes on\n>>>>>> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n>>>>>> +\t */\n>>>>>> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n>>>>>> +\n>>>>>> +\t/*\n>>>>>> +\t * Select the sensor format, use either the best fit to the configured\n>>>>>> +\t * format or a specific sensor format, when getFormat would choose a\n>>>>>> +\t * resolution that surpasses the ISP maximum.\n>>>>>> +\t */\n>>>>>> +\tSize maxSensorSize;\n>>>>>> +\tfor (const Size &size : sensor->sizes()) {\n>>>>>> +\t\tif (size.width > ispMaximum.width ||\n>>>>>> +\t\t size.height > ispMaximum.height)\n>>>>>> +\t\t\tcontinue;\n>>>>>\n>>>>> This makes me think we need better comparison functions for the Size\n>>>>> class. Maybe a Size::contains() function ? It doesn't have to be part of\n>>>>> this series.\n>>>>>\n>>>>>> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n>>>>>> +\t}\n>>>>>\n>>>>> Missing blank line here too.\n>>>>>\n>>>>> Could we move all the code above to\n>>>>> PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n>>>>> RkISP1CameraData, to avoid doing the calculation every time validate()\n>>>>> is called ?\n>>>>>\n>>>>>\n>>>>>> +\tSize maxConfigSize;\n>>>>>> \tfor (const StreamConfiguration &cfg : config_)\n>>>>>> -\t\tmaxSize = std::max(maxSize, cfg.size);\n>>>>>> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n>>>>>> +\n>>>>>> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n>>>>>> +\t maxConfigSize.width <= maxSensorSize.width)\n>>>>>> +\t\tmaxSensorSize = maxConfigSize;\n>>>>>> \n>>>>>> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n>>>>\n>>>> I wonder if it won't be an easier solution to add another optional parameter\n>>>> to the getFormat method of the sensor that limits the size that the sensor\n>>>> should return. This might benefit other pipline-handlers as well where\n>>>> a sensor is connected to an entity that has a maximal size it can accept.\n>>>> In rkisp1 all the above code could be replaced by just adding\n>>>> Size(4032,3024) as another parameter to getFormat.\n>>>>\n>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n>>>>>> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n>>>>>> -\t\t\t\t\t maxSize);\n>>>>>> +\t\t\t\t\t maxSensorSize);\n>>>>>> \tif (sensorFormat_.size.isNull())\n>>>>>> \t\tsensorFormat_.size = sensor->resolution();\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 3A2CEBD80E\n\tfor ;\n\tMon, 15 Mar 2021 16:52:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E14568D46;\n\tMon, 15 Mar 2021 17:52:22 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 589F768D3E\n\tfor ;\n\tMon, 15 Mar 2021 17:52:20 +0100 (CET)","from [IPv6:2003:c7:cf38:3800:d4a:c7b9:c2ef:774c]\n\t(p200300c7cf3838000d4ac7b9c2ef774c.dip0.t-ipconnect.de\n\t[IPv6:2003:c7:cf38:3800:d4a:c7b9:c2ef:774c])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: dafna)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id F1A5E1F45633;\n\tMon, 15 Mar 2021 16:52:19 +0000 (GMT)"],"To":"Laurent Pinchart ","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t\n\t\n\t<6274ed99-d62a-a995-3b79-4d0f76c533d6@collabora.com>\n\t","From":"Dafna Hirschfeld ","Message-ID":"<8f1b35b1-feb8-7e04-095a-d70c8db315d3@collabora.com>","Date":"Mon, 15 Mar 2021 17:52:14 +0100","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":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15735,"web_url":"https://patchwork.libcamera.org/comment/15735/","msgid":"<20210317070716.mjhssi4je7irynqa@basti-TUXEDO-Book-XA1510>","date":"2021-03-17T07:07:16","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":78,"url":"https://patchwork.libcamera.org/api/people/78/","name":"Sebastian Fricke","email":"sebastian.fricke@posteo.net"},"content":"Hello Dafna, Laurent and Heiko,\n\nthank you all for your reviews, I have looked further into the problem\nand would like to share my thoughts with you.\n\nComments below...\n\nOn 15.03.2021 17:52, Dafna Hirschfeld wrote:\n>\n>\n>On 12.03.21 22:31, Laurent Pinchart wrote:\n>>Hi Dafna,\n>>\n>>On Tue, Mar 02, 2021 at 09:16:04AM +0100, Dafna Hirschfeld wrote:\n>>>On 02.03.21 03:39, Laurent Pinchart wrote:\n>>>>On Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:\n>>>>>On 28.02.21 16:49, Laurent Pinchart wrote:\n>>>>>>On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n>>>>>>>This patch fixes a mismatch of image formats during the pipeline\n>>>>>>>creation of the RkISP1. The mismatch happens because the current code\n>>>>>>>does not check if the configured format exceeds the maximum viable\n>>>>>>>resolution of the ISP.\n>>>>>>>\n>>>>>>>Make sure to use a sensor format resolution that is smaller or equal to\n>>>>>>>the maximum allowed resolution for the RkISP1. The maximum resolution\n>>>>>>>is defined within the `rkisp1-common.h` file as:\n>>>>>>>define RKISP1_ISP_MAX_WIDTH 4032\n>>>>>>>define RKISP1_ISP_MAX_HEIGHT 3024\n>>>>>>>\n>>>>>>>Enumerate the frame-sizes of the ISP entity and compare the maximum with\n>>>>>>>the configured resolution.\n>>>>>>>\n>>>>>>>This means that some camera sensors can never operate with their maximum\n>>>>>>>resolution, for example on my OV13850 camera sensor, there are two\n>>>>>>>possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n>>>>>>>never be picked as it surpasses the maximum of the ISP.\n>>>>>>\n>>>>>>It would have been nice if the ISP had an input crop rectangle :-S It\n>>>>>>would have allowed us to crop the input image a little bit to 4032x2992\n>>>>>>(keeping the same aspect ratio) or 4032x3024 (4:3). Just\n>>>>>>double-checking, is there indeed no way to crop at the input, or could\n>>>>>>it be that it's not implemented in the driver ? I can't find the\n>>>>>>4032x3024 limit in the documentation I have access to.\n>>>>>\n>>>>>The isp does support cropping on the sink pad.\n>>>>>But currently the function v4l2_subdev_link_validate_default compares\n>>>>>the dimensions defined in v4l2_subdev_format which are not the crop\n>>>>>dimensions but the ones set by set_fmt. Is that wrong?\n>>>>\n>>>>I think so. If the ISP supports a larger size than 4032x3024 before\n>>>>cropping, it should accept that on its sink pad, with the sink crop\n>>>>rectangle being adjusted to never be larger than 4032x3024 (for instance\n>>>>when userspace sets the format on the sink pad, the crop rectangle could\n>>>>be automatically set to the same size, clamped to 4032x3024 and\n>>>>centered). Userspace can then adjust the crop rectangle further if\n>>>>necessary.\n>>>\n>>>In rkisp1-isp.c, there is diagram of the cropping regions.\n>>>It says that the 4032x3024 limit is 'for black level'.\n>>>Does that means that some sensors might send frames with black pixels in\n>>>the edges that need to be cropped?\n>>\n>>In this context, I believe that black level refers to black level\n>>correction (BLC in short), which is the process of subtracting a fixed\n>>value from all pixels to account for leakage currents and other\n>>parasitic effects in the sensor that makes fully black pixels have a\n>>non-zero value. Sensors typically have a set of lines before the image\n>>that is physically covered, and those lines can then be transmitted over\n>>CSI-2. The average black level will then be computed on the SoC side\n>>(either using the CPU, or in the ISP if it supports doing so), and\n>>configured in the ISP to subtract it from all pixels. The black lines\n>>are cropped out of what the ISP processes further down the pipeline.\n>\n>The number of black/invalid lines depends on the sensor right?\n>So userspace should adjust the cropping according to the sensor.\n>If so then why would we want to always clamp to 4032x3024 ?\n>Or should it just be the initial value and userspace can then increase\n>the crop size?\n>\n>>\n>>>The TRM says \"Maximum input resolution of 4416x3312 pixels\"\n>>>The datasheet then shows that the default values are 4096x3072 for both the\n>>>input resolution (ISP_ACQ) and for the corp in the sink (ISP_OUT).\n>>>\n>>>So from the docs at least it sound possible to do as you suggested,\n>>>limit the input to 4416x3312 and then always set a crop with maximum\n>>>value 4032x3024\n>>\n>>Sounds like it's worth a try at least :-)\n\nDafna advised me to try and set the MAX and MIN size simply to\n4416x3312, so I did and here are the resulting images.\n\nI captured one video with these settings:\n`LIBCAMERA_LOG_LEVELS=0 cam --camera=1 --capture=50 --file=/tmp/out_video -s height=600,width=800,pixelformat=NV12,role=video`\n\nlibcamera then sets the camera pipeline like this:\n```\n[0:35:57.915030227] [6669] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 2112x1568-SBGGR10_1X10\n[0:35:57.915069602] [6669] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 2112x1568-SBGGR10_1X10\n[0:35:57.915125018] [6669] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568\n[0:35:57.915152143] [6669] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568\n[0:35:57.915183643] [6669] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568\n[0:35:57.915234684] [6669] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568\n[0:35:57.915262101] [6669] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 800x600-YUYV8_2X8\n[0:35:57.915284850] [6669] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 800x600-YUYV8_1_5X8\n```\n\nAnd another video with these settings:\n`cam --camera=1 --capture=50 --file=/tmp/out_video -s height=2160,width=3840,pixelformat=NV12,role=video`\n\nlibcamera sets the camera pipeline like this:\n```\n[0:52:25.445115742] [8997] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10\n[0:52:25.445158617] [8997] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10\n[0:52:25.445214616] [8997] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136\n[0:52:25.445242616] [8997] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n[0:52:25.445274991] [8997] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n[0:52:25.445325449] [8997] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n[0:52:25.445351116] [8997] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3840x2160-YUYV8_2X8\n[0:52:25.445372990] [8997] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3840x2160-YUYV8_1_5X8\n```\n\nHere are the results of both captures (I have taken roughly the same\nframe number):\nhttps://imgur.com/a/EFtEmB9\n\nThe image quality of the higher resolution image is obviously better and\nit is also quite a lot brighter. I was not able to detect any defect\npixels in the image so far.\nBut I noticed the following:\nMy sensor has a Black Level Calibration (BLC) register (@ 0x5001),\nusually, BLC is enabled. If I turn it off and try to capture with a\nsensor resolution of 4224x3136, then the pipeline can still be\ncreated, validated and the buffers are queued, but it seems like the sensor\ndoesn't start anymore. When I use the lower resolution mode of the sensor\n(2112x1568), while deactivating BLC everything works just fine.\nSo, something happens when BLC is off when the resolution is greater\nthan 4032x3024. (Sadly no part of the camera pipeline actually throws an\nerror)\n\nHere is the debug log from cam:\n```\n[0:11:23.311130186] [4131] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10\n[0:11:23.311269310] [4131] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10\n[0:11:23.311451309] [4131] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136\n[0:11:23.311571766] [4131] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n[0:11:23.311728682] [4131] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n[0:11:23.311918555] [4131] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n[0:11:23.312134679] [4131] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8\n[0:11:23.312276428] [4131] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 1920x1920-YUYV8_1_5X8\n[0:11:23.313918792] [4131] INFO IPARkISP1 rkisp1.cpp:120 Exposure: 4-3324 Gain: 16-248\n[0:11:23.314731370] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Analogue Gain to 16 at index 1\n[0:11:23.314913660] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Exposure to 4 at index 1\n[0:11:23.336376391] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.\n[0:11:23.337981130] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 0 buffers requested.\n[0:11:23.338467627] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n[0:11:23.339042498] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n[0:11:23.339212247] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n[0:11:23.339364496] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n[0:11:23.339509453] [4130] DEBUG Camera camera.cpp:1029 Starting capture\n[0:11:23.340191657] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video3[15:out]: 4 buffers requested.\n[0:11:23.341328566] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video2[14:cap]: 4 buffers requested.\n[0:11:23.344065548] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.\n[0:11:23.344281088] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1351 /dev/video0[17:cap]: Prepared to import 4 buffers\nCapture 5 frames\n[0:11:23.346221533] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 0\n[0:11:23.346598364] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 0\n[0:11:23.346800779] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 0\n[0:11:23.347141443] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 1\n[0:11:23.347319651] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 1\n[0:11:23.347473941] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 1\n[0:11:23.347759189] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 2\n[0:11:23.347921938] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 2\n[0:11:23.348139520] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 2\n[0:11:23.404159310] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 3\n[0:11:23.404329934] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 3\n[0:11:23.404401100] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 3\n[0:11:23.404646390] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video3[15:out]: Dequeuing buffer 0\n^CExiting\n```\n\nConclusion:\nAt first glance, I thought it might be fine to just increase the\nresolution limits, but after seeing that this only works when the sensor\nperforms black level calibration, I think that it would be unwise to\nperform this change.\n\nI am now working on the initial idea to crop the sink pad of the ISP\ndown to 4032x3024, if and only if the input resolution is greater than\n4032x3024 and smaller or equal to 4416x3312 (because I don't want to\ncreate a crop to 4032x3024 when the input resolution is for example\n2112x1568). That crop is automatically propagated to the source pad of\nthe ISP within `rkisp1_isp_set_sink_crop` and within\n`rkisp1_isp_set_src_fmt` we use the crop resolution as format\nresolution. I will post that patch soon to the mailing list.\n\n>>\n>>>>>>>Signed-off-by: Sebastian Fricke \n>>>>>>>---\n>>>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n>>>>>>> 1 file changed, 31 insertions(+), 4 deletions(-)\n>>>>>>>\n>>>>>>>diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>>>>index 50eaa6a4..56a406c1 100644\n>>>>>>>--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>>>>+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>>>>@@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>>>>> \t\treturn Invalid;\n>>>>>>> \t}\n>>>>>>>-\t/* Select the sensor format. */\n>>>>>>>-\tSize maxSize;\n>>>>>>>+\t/* Get the ISP resolution limits */\n>>>>>>>+\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n>>>>>>\n>>>>>>Related to 1/2, note that you don't necessarily need to store the ISP\n>>>>>>pointer in RkISP1CameraData. You can access the pipeline handler here:\n>>>>>>\n>>>>>>\tPipelineHandlerRkISP1 *pipe =\n>>>>>>\t\tstatic_cast(data_->pipe_);\n>>>>>>\tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n>>>>>>\n>>>>>>>+\tif (ispFormats.empty()) {\n>>>>>>>+\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n>>>>>>>+\t\treturn Invalid;\n>>>>>>>+\t}\n>>>>>>\n>>>>>>Missing blank line.\n>>>>>>\n>>>>>>>+\t/*\n>>>>>>>+\t * The maximum resolution is identical for all media bus codes on\n>>>>>>>+\t * the RkISP1 isp entity. Therefore take the first available resolution.\n>>>>>>>+\t */\n>>>>>>>+\tSize ispMaximum = ispFormats.begin()->second[0].max;\n>>>>>>>+\n>>>>>>>+\t/*\n>>>>>>>+\t * Select the sensor format, use either the best fit to the configured\n>>>>>>>+\t * format or a specific sensor format, when getFormat would choose a\n>>>>>>>+\t * resolution that surpasses the ISP maximum.\n>>>>>>>+\t */\n>>>>>>>+\tSize maxSensorSize;\n>>>>>>>+\tfor (const Size &size : sensor->sizes()) {\n>>>>>>>+\t\tif (size.width > ispMaximum.width ||\n>>>>>>>+\t\t size.height > ispMaximum.height)\n>>>>>>>+\t\t\tcontinue;\n>>>>>>\n>>>>>>This makes me think we need better comparison functions for the Size\n>>>>>>class. Maybe a Size::contains() function ? It doesn't have to be part of\n>>>>>>this series.\n\nI would love to work on that right after this patch and the connected\nkernel patch :).\n\n>>>>>>\n>>>>>>>+\t\tmaxSensorSize = std::max(maxSensorSize, size);\n>>>>>>>+\t}\n>>>>>>\n>>>>>>Missing blank line here too.\n>>>>>>\n>>>>>>Could we move all the code above to\n>>>>>>PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n>>>>>>RkISP1CameraData, to avoid doing the calculation every time validate()\n>>>>>>is called ?\n\nYes, that is a good idea, and actually when I think about it the most\nlogical location. I hope that my use case will be handled by the kernel\npatch, but I will still post this patch in case a camera sensor comes\nalong with a resolution greater than 4416x3312. I will test the\nlibcamera patch without the kernel patch to make sure that it works. But\nit would be great if someone could test the patch in combination with\nthe kernel patch, with an appropriate sensor. (I would also be open to\nany recommendations as I don't know if such a sensor exists for my\nplatform).\n\n>>>>>>\n>>>>>>\n>>>>>>>+\tSize maxConfigSize;\n>>>>>>> \tfor (const StreamConfiguration &cfg : config_)\n>>>>>>>-\t\tmaxSize = std::max(maxSize, cfg.size);\n>>>>>>>+\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n>>>>>>>+\n>>>>>>>+\tif (maxConfigSize.height <= maxSensorSize.height &&\n>>>>>>>+\t maxConfigSize.width <= maxSensorSize.width)\n>>>>>>>+\t\tmaxSensorSize = maxConfigSize;\n>>>>>>> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n>>>>>\n>>>>>I wonder if it won't be an easier solution to add another optional parameter\n>>>>>to the getFormat method of the sensor that limits the size that the sensor\n>>>>>should return. This might benefit other pipline-handlers as well where\n>>>>>a sensor is connected to an entity that has a maximal size it can accept.\n>>>>>In rkisp1 all the above code could be replaced by just adding\n>>>>>Size(4032,3024) as another parameter to getFormat.\n\nI could add that in another patch, but I believe that if I don't require\nthat for the rkisp1, I would first wait for an actual use case to\nappear.\n\n>>>>>\n>>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n>>>>>>>@@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n>>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n>>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n>>>>>>>-\t\t\t\t\t maxSize);\n>>>>>>>+\t\t\t\t\t maxSensorSize);\n>>>>>>> \tif (sensorFormat_.size.isNull())\n>>>>>>> \t\tsensorFormat_.size = sensor->resolution();\n>>\n\nGreetings,\nSebastian","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 DFCB1BD80C\n\tfor ;\n\tWed, 17 Mar 2021 07:07:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E94D668D64;\n\tWed, 17 Mar 2021 08:07:20 +0100 (CET)","from mout01.posteo.de (mout01.posteo.de [185.67.36.65])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBFC568D5E\n\tfor ;\n\tWed, 17 Mar 2021 08:07:18 +0100 (CET)","from submission (posteo.de [89.146.220.130]) \n\tby mout01.posteo.de (Postfix) with ESMTPS id 1C938160063\n\tfor ;\n\tWed, 17 Mar 2021 08:07:18 +0100 (CET)","from customer (localhost [127.0.0.1])\n\tby submission (posteo.de) with ESMTPSA id 4F0h6s15Ynz9rxL;\n\tWed, 17 Mar 2021 08:07:17 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=posteo.net header.i=@posteo.net\n\theader.b=\"dA3/1Vie\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017;\n\tt=1615964838; bh=QojUQxGpZgcY8fTv3y9rLkenV01cvVSor0K9Z7J1hXg=;\n\th=Date:From:To:Cc:Subject:From;\n\tb=dA3/1VieXstnOSekUgdh4L9XjNAfn+4Q0/SkNyOhTzu2TxTPWuxb7wJM1184Au5Ot\n\txpyGTeXg5vEajL8xn+R/iY+JoJ4t5iflHvYm8sAQurNjeifcQ+UpZOJdiVsGRqYL8g\n\tg0SQoNS1QD/7BtLpiG5F8Xe84t3dkYvk/mWUs5lPsa+dGiYudRhs4HeC1msjYGZnYe\n\tYpnCMBV6AWxwE0KyZDn8Ru88GFz6ted8ajfQMyoF1vl//caI7+ulvfNtTSC32yb3T8\n\tXDbV78EeuZSSM3glGydc3M6Z6HvWmyvihfouZy1FMt0sdgXoCSv9PKbpEUe2Df2bTX\n\tdZOEdkwF43+EA==","Date":"Wed, 17 Mar 2021 08:07:16 +0100","From":"Sebastian Fricke ","To":"Dafna Hirschfeld ","Message-ID":"<20210317070716.mjhssi4je7irynqa@basti-TUXEDO-Book-XA1510>","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t\n\t\n\t<6274ed99-d62a-a995-3b79-4d0f76c533d6@collabora.com>\n\t\n\t<8f1b35b1-feb8-7e04-095a-d70c8db315d3@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<8f1b35b1-feb8-7e04-095a-d70c8db315d3@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15767,"web_url":"https://patchwork.libcamera.org/comment/15767/","msgid":"<20210319071901.rbecfp5z6zxavnit@basti-TUXEDO-Book-XA1510>","date":"2021-03-19T07:19:01","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":78,"url":"https://patchwork.libcamera.org/api/people/78/","name":"Sebastian Fricke","email":"sebastian.fricke@posteo.net"},"content":"Hello Laurent, Dafna and Helen,\n\nI have investigated this issue and have written some thoughts below.\n\nOn 01.03.2021 08:48, Dafna Hirschfeld wrote:\n>Hi\n>\n>On 28.02.21 16:49, Laurent Pinchart wrote:\n>>Hi Sebastian,\n>>\n>>Thank you for the patch.\n>>\n>>On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n>>>This patch fixes a mismatch of image formats during the pipeline\n>>>creation of the RkISP1. The mismatch happens because the current code\n>>>does not check if the configured format exceeds the maximum viable\n>>>resolution of the ISP.\n>>>\n>>>Make sure to use a sensor format resolution that is smaller or equal to\n>>>the maximum allowed resolution for the RkISP1. The maximum resolution\n>>>is defined within the `rkisp1-common.h` file as:\n>>>define RKISP1_ISP_MAX_WIDTH 4032\n>>>define RKISP1_ISP_MAX_HEIGHT 3024\n>>>\n>>>Enumerate the frame-sizes of the ISP entity and compare the maximum with\n>>>the configured resolution.\n>>>\n>>>This means that some camera sensors can never operate with their maximum\n>>>resolution, for example on my OV13850 camera sensor, there are two\n>>>possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n>>>never be picked as it surpasses the maximum of the ISP.\n>>\n>>It would have been nice if the ISP had an input crop rectangle :-S It\n>>would have allowed us to crop the input image a little bit to 4032x2992\n>>(keeping the same aspect ratio) or 4032x3024 (4:3). Just\n>>double-checking, is there indeed no way to crop at the input, or could\n>>it be that it's not implemented in the driver ? I can't find the\n>>4032x3024 limit in the documentation I have access to.\n>\n>The isp does support cropping on the sink pad.\n>But currently the function v4l2_subdev_link_validate_default compares\n>the dimensions defined in v4l2_subdev_format which are not the crop\n>dimensions but the ones set by set_fmt. Is that wrong?\n\nI have looked into the code and I noticed that the cropping ability of\nthe ISP sink pad is more like a dummy functionality. You are able to\nset a crop that is smaller than the format for the sink pad, but there\nis a chain of events that make this condition invalid. When\n`rkisp1_isp_set_sink_fmt` is called it automatically calls\n`rkisp1_isp_set_sink_crop` with the last used crop for the pad\n(0,0,800,600) by default. Within `set_sink_crop` the crop is aligned to\nthe `sink_fmt` so that it is within the bounds of the new format. Then\nit goes on to call `rkisp1_isp_set_src_crop` with the last used crop on\nthe source pad, it maps the `src_crop` into the `sink_crop`. And finally\n`rkisp1_isp_set_src_fmt` is called, which sets the width and height of\nthe format with the width and height of the crop.\nWe validate the links with `v4l2_subdev_link_validate_default`, which\nchecks if the formats of the pads are equal.\n\nTherefore I can state the following:\n- the sink pad crop is bounded by the sink pad format\n- the source pad crop is bounded by the sink pad crop\n- the source pad format is bounded by the source pad format\nMeaning: when the sink pad format != sink pad crop, then the sink pad format\nand source pad format can never be equal. And therefore can never be\nvalidated.\n\nThe rockchip documentation of the ISP1 driver (http://opensource.rock-chips.com/wiki_Rockchip-isp1#V4l2_view),\nmentions the following statement for the sink pad of the ISP:\n\"the size should be equal/less than sensor input size.\"\nand for the source pad of the ISP:\n\"The size should be equal/less than sink pad size.\"\n\nThis means from the ISP's point of view, the following pad configuration\nshould probably work:\n```\n- entity 1: rkisp1_isp (4 pads, 5 links)\n ...\n\tpad0: Sink\n\t\t[fmt:SBGGR10_1X10/4224x3136 field:none\n\t\t crop.bounds:(0,0)/4224x3136\n\t\t crop:(96,72)/4032x2992]\n\t\t<- \"ov13850 1-0010\":0 [ENABLED]\n ...\n\tpad2: Source\n\t\t[fmt:YUYV8_2X8/4032x2992 field:none\n\t\t crop.bounds:(0,0)/4032x2992\n\t\t crop:(0,0)/4032x2992]\n\t\t-> \"rkisp1_resizer_mainpath\":0 [ENABLED]\n\t\t-> \"rkisp1_resizer_selfpath\":0 []\n ...\n- entity 6: rkisp1_resizer_mainpath (2 pads, 2 links)\n ...\n\tpad0: Sink\n\t\t[fmt:YUYV8_2X8/4032x2992 field:none\n\t\t crop.bounds:(0,0)/4032x2992\n\t\t crop:(0,0)/4032x2992]\n\t\t<- \"rkisp1_isp\":2 [ENABLED]\n\tpad1: Source\n\t\t[fmt:YUYV8_2X8/1920x1920 field:none]\n\t\t-> \"rkisp1_mainpath\":0 [ENABLED,IMMUTABLE]\n- entity 28: ov13850 1-0010 (1 pad, 1 link)\n ...\n\tpad0: Source\n\t\t[fmt:SBGGR10_1X10/4224x3136@20000/150000 field:none\n\t\t crop.bounds:(16,8)/4224x3136\n\t\t crop:(16,8)/4224x3136]\n\t\t-> \"rkisp1_isp\":0 [ENABLED]\n```\n\nBut the problem is that this not how the link validation currently works,\nwhen the format sizes are not equal `v4l2_subdev_link_validate_default`,\nwill definitely fail.\n\nNow, I have 3 ideas in mind for how to continue with this issue.\n1. We could modify the callback to validate media links, that would\nexplicitly handle ISP sink and source differently, and check if the crops\nalign when the sink format size is greater than 4032x3024. I think that\nthis option is quite ugly and too specific.\n2. We could add a new function similar to `v4l2_subdev_link_validate_default`,\nthat performs the same checks but additionally also checks if the crop\nof a sink pad is equal to the format of a source pad when the format\nsize validation failed. I am not too sure about this either.\n3. We don't touch this part of the code and I fix the issue only in\nlibcamera and the 4224x3136 mode of my camera cannot be used :(.\n\nI am not too happy with any of the solutions, that I have proposed, I\nwould love to hear your thoughts and ideas.\n\nGreetings,\nSebastian\n>\n>>\n>>>Signed-off-by: Sebastian Fricke \n>>>---\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n>>> 1 file changed, 31 insertions(+), 4 deletions(-)\n>>>\n>>>diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>index 50eaa6a4..56a406c1 100644\n>>>--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>@@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> \t\treturn Invalid;\n>>> \t}\n>>>-\t/* Select the sensor format. */\n>>>-\tSize maxSize;\n>>>+\t/* Get the ISP resolution limits */\n>>>+\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n>>\n>>Related to 1/2, note that you don't necessarily need to store the ISP\n>>pointer in RkISP1CameraData. You can access the pipeline handler here:\n>>\n>>\tPipelineHandlerRkISP1 *pipe =\n>>\t\tstatic_cast(data_->pipe_);\n>>\tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n>>\n>>>+\tif (ispFormats.empty()) {\n>>>+\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n>>>+\t\treturn Invalid;\n>>>+\t}\n>>\n>>Missing blank line.\n>>\n>>>+\t/*\n>>>+\t * The maximum resolution is identical for all media bus codes on\n>>>+\t * the RkISP1 isp entity. Therefore take the first available resolution.\n>>>+\t */\n>>>+\tSize ispMaximum = ispFormats.begin()->second[0].max;\n>>>+\n>>>+\t/*\n>>>+\t * Select the sensor format, use either the best fit to the configured\n>>>+\t * format or a specific sensor format, when getFormat would choose a\n>>>+\t * resolution that surpasses the ISP maximum.\n>>>+\t */\n>>>+\tSize maxSensorSize;\n>>>+\tfor (const Size &size : sensor->sizes()) {\n>>>+\t\tif (size.width > ispMaximum.width ||\n>>>+\t\t size.height > ispMaximum.height)\n>>>+\t\t\tcontinue;\n>>\n>>This makes me think we need better comparison functions for the Size\n>>class. Maybe a Size::contains() function ? It doesn't have to be part of\n>>this series.\n>>\n>>>+\t\tmaxSensorSize = std::max(maxSensorSize, size);\n>>>+\t}\n>>\n>>Missing blank line here too.\n>>\n>>Could we move all the code above to\n>>PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n>>RkISP1CameraData, to avoid doing the calculation every time validate()\n>>is called ?\n>>\n>>\n>>>+\tSize maxConfigSize;\n>>> \tfor (const StreamConfiguration &cfg : config_)\n>>>-\t\tmaxSize = std::max(maxSize, cfg.size);\n>>>+\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n>>>+\n>>>+\tif (maxConfigSize.height <= maxSensorSize.height &&\n>>>+\t maxConfigSize.width <= maxSensorSize.width)\n>>>+\t\tmaxSensorSize = maxConfigSize;\n>>> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n>\n>I wonder if it won't be an easier solution to add another optional parameter\n>to the getFormat method of the sensor that limits the size that the sensor\n>should return. This might benefit other pipline-handlers as well where\n>a sensor is connected to an entity that has a maximal size it can accept.\n>In rkisp1 all the above code could be replaced by just adding\n>Size(4032,3024) as another parameter to getFormat.\n>\n>Thanks,\n>Dafna\n>\n>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n>>>@@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n>>> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n>>> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n>>>-\t\t\t\t\t maxSize);\n>>>+\t\t\t\t\t maxSensorSize);\n>>> \tif (sensorFormat_.size.isNull())\n>>> \t\tsensorFormat_.size = sensor->resolution();\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 971B1C32E1\n\tfor ;\n\tFri, 19 Mar 2021 07:19:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC2F068D51;\n\tFri, 19 Mar 2021 08:19:05 +0100 (CET)","from mout01.posteo.de (mout01.posteo.de [185.67.36.65])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED21C605B1\n\tfor ;\n\tFri, 19 Mar 2021 08:19:03 +0100 (CET)","from submission (posteo.de [89.146.220.130]) \n\tby mout01.posteo.de (Postfix) with ESMTPS id 6926916005F\n\tfor ;\n\tFri, 19 Mar 2021 08:19:03 +0100 (CET)","from customer (localhost [127.0.0.1])\n\tby submission (posteo.de) with ESMTPSA id 4F1wHV2bhXz9rxK;\n\tFri, 19 Mar 2021 08:19:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=posteo.net header.i=@posteo.net\n\theader.b=\"djpgUT0H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017;\n\tt=1616138343; bh=46TnlMiDBZekwmi5xwwghfdA6ez+kWR/+5pP/X+/6Rc=;\n\th=Date:From:To:Cc:Subject:From;\n\tb=djpgUT0HOhf+NSfm1sraT3mSyTqAH5hH91VOKdp4c2Emgfhg1Puc6Df4y8Tx05fjt\n\tm1tP1QIUkTYiAF8vznNHVUie0FX3OsXN3OBfQHF99IlGH3Kx6IpboTPjCCG11KAxib\n\tA+c8OxSK1LPmItwLhqRLwtmKgHY2EjZ8I4/zWhwyiiy2XdgntSQ2jJ5cfNJnfUWTSJ\n\tC280AOruH+3doWD7Fs4ILbDSJhv/5YNZXE87O4yXyIp4BPSGZhoCT2zZE8muwcFaOm\n\t76v0M+A4rwgG36WrjqxQZCFh+kBD22RP1PKMDzUHQdN3nQ3DCFeu5gX4gATBGy4M4K\n\tgdB5Bqz0oFQZA==","Date":"Fri, 19 Mar 2021 08:19:01 +0100","From":"Sebastian Fricke ","To":"Dafna Hirschfeld ","Message-ID":"<20210319071901.rbecfp5z6zxavnit@basti-TUXEDO-Book-XA1510>","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15828,"web_url":"https://patchwork.libcamera.org/comment/15828/","msgid":"<820cec27-63c9-02c4-60a1-0ff9af9dd283@collabora.com>","date":"2021-03-23T11:56:11","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"On 19.03.21 08:19, Sebastian Fricke wrote:\n> Hello Laurent, Dafna and Helen,\n> \n> I have investigated this issue and have written some thoughts below.\n> \n> On 01.03.2021 08:48, Dafna Hirschfeld wrote:\n>> Hi\n>>\n>> On 28.02.21 16:49, Laurent Pinchart wrote:\n>>> Hi Sebastian,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n>>>> This patch fixes a mismatch of image formats during the pipeline\n>>>> creation of the RkISP1. The mismatch happens because the current code\n>>>> does not check if the configured format exceeds the maximum viable\n>>>> resolution of the ISP.\n>>>>\n>>>> Make sure to use a sensor format resolution that is smaller or equal to\n>>>> the maximum allowed resolution for the RkISP1. The maximum resolution\n>>>> is defined within the `rkisp1-common.h` file as:\n>>>> define RKISP1_ISP_MAX_WIDTH 4032\n>>>> define RKISP1_ISP_MAX_HEIGHT 3024\n>>>>\n>>>> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n>>>> the configured resolution.\n>>>>\n>>>> This means that some camera sensors can never operate with their maximum\n>>>> resolution, for example on my OV13850 camera sensor, there are two\n>>>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n>>>> never be picked as it surpasses the maximum of the ISP.\n>>>\n>>> It would have been nice if the ISP had an input crop rectangle :-S It\n>>> would have allowed us to crop the input image a little bit to 4032x2992\n>>> (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n>>> double-checking, is there indeed no way to crop at the input, or could\n>>> it be that it's not implemented in the driver ? I can't find the\n>>> 4032x3024 limit in the documentation I have access to.\n>>\n>> The isp does support cropping on the sink pad.\n>> But currently the function v4l2_subdev_link_validate_default compares\n>> the dimensions defined in v4l2_subdev_format which are not the crop\n>> dimensions but the ones set by set_fmt. Is that wrong?\n> \n> I have looked into the code and I noticed that the cropping ability of\n> the ISP sink pad is more like a dummy functionality. You are able to\n> set a crop that is smaller than the format for the sink pad, but there\n> is a chain of events that make this condition invalid. When\n> `rkisp1_isp_set_sink_fmt` is called it automatically calls\n> `rkisp1_isp_set_sink_crop` with the last used crop for the pad\n> (0,0,800,600) by default. Within `set_sink_crop` the crop is aligned to\n> the `sink_fmt` so that it is within the bounds of the new format. Then\n> it goes on to call `rkisp1_isp_set_src_crop` with the last used crop on\n> the source pad, it maps the `src_crop` into the `sink_crop`. And finally\n> `rkisp1_isp_set_src_fmt` is called, which sets the width and height of\n> the format with the width and height of the crop.\n> We validate the links with `v4l2_subdev_link_validate_default`, which\n> checks if the formats of the pads are equal.\n> \n> Therefore I can state the following:\n> - the sink pad crop is bounded by the sink pad format\n> - the source pad crop is bounded by the sink pad crop\n> - the source pad format is bounded by the source pad format\n> Meaning: when the sink pad format != sink pad crop, then the sink pad format\n> and source pad format can never be equal. And therefore can never be\n> validated.\n\nHi, note that the function v4l2_subdev_link_validate_default compares the pads of a link, that is, the two pads belong\nto different entities in both side of the link. So inside the isp entity it is valid to have \"sink_fmt > sink_crop > src_fmt > src_crop\"\nsince the src_* and sink_* values belong to different pads of the same entity and not to pads of the same link.\n\n> \n> The rockchip documentation of the ISP1 driver (http://opensource.rock-chips.com/wiki_Rockchip-isp1#V4l2_view),\n> mentions the following statement for the sink pad of the ISP:\n> \"the size should be equal/less than sensor input size.\"\n> and for the source pad of the ISP:\n> \"The size should be equal/less than sink pad size.\"\n> \n> This means from the ISP's point of view, the following pad configuration\n> should probably work:\n> ```\n> - entity 1: rkisp1_isp (4 pads, 5 links)\n>     ...\n>     pad0: Sink\n>         [fmt:SBGGR10_1X10/4224x3136 field:none\n>          crop.bounds:(0,0)/4224x3136\n>          crop:(96,72)/4032x2992]\n>         <- \"ov13850 1-0010\":0 [ENABLED]\n>     ...\n>     pad2: Source\n>         [fmt:YUYV8_2X8/4032x2992 field:none\n>          crop.bounds:(0,0)/4032x2992\n>          crop:(0,0)/4032x2992]\n>         -> \"rkisp1_resizer_mainpath\":0 [ENABLED]\n>         -> \"rkisp1_resizer_selfpath\":0 []\n>     ...\n> - entity 6: rkisp1_resizer_mainpath (2 pads, 2 links)\n>     ...\n>     pad0: Sink\n>         [fmt:YUYV8_2X8/4032x2992 field:none\n>          crop.bounds:(0,0)/4032x2992\n>          crop:(0,0)/4032x2992]\n>         <- \"rkisp1_isp\":2 [ENABLED]\n>     pad1: Source\n>         [fmt:YUYV8_2X8/1920x1920 field:none]\n>         -> \"rkisp1_mainpath\":0 [ENABLED,IMMUTABLE]\n> - entity 28: ov13850 1-0010 (1 pad, 1 link)\n>     ...\n>     pad0: Source\n>         [fmt:SBGGR10_1X10/4224x3136@20000/150000 field:none\n>          crop.bounds:(16,8)/4224x3136\n>          crop:(16,8)/4224x3136]\n>         -> \"rkisp1_isp\":0 [ENABLED]\n\nThis configuration seems valid to me.\nDo you get EPIPE when streaming with this configuration?\n\nThanks,\nDafna\n\n> ```\n> \n> But the problem is that this not how the link validation currently works,\n> when the format sizes are not equal `v4l2_subdev_link_validate_default`,\n> will definitely fail.\n> \n> Now, I have 3 ideas in mind for how to continue with this issue.\n> 1. We could modify the callback to validate media links, that would\n> explicitly handle ISP sink and source differently, and check if the crops\n> align when the sink format size is greater than 4032x3024. I think that\n> this option is quite ugly and too specific.\n> 2. We could add a new function similar to `v4l2_subdev_link_validate_default`,\n> that performs the same checks but additionally also checks if the crop\n> of a sink pad is equal to the format of a source pad when the format\n> size validation failed. I am not too sure about this either.\n> 3. We don't touch this part of the code and I fix the issue only in\n> libcamera and the 4224x3136 mode of my camera cannot be used :(.\n> \n> I am not too happy with any of the solutions, that I have proposed, I\n> would love to hear your thoughts and ideas.\n> \n> Greetings,\n> Sebastian\n>>\n>>>\n>>>> Signed-off-by: Sebastian Fricke \n>>>> ---\n>>>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n>>>>  1 file changed, 31 insertions(+), 4 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> index 50eaa6a4..56a406c1 100644\n>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>>          return Invalid;\n>>>>      }\n>>>> -    /* Select the sensor format. */\n>>>> -    Size maxSize;\n>>>> +    /* Get the ISP resolution limits */\n>>>> +    V4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n>>>\n>>> Related to 1/2, note that you don't necessarily need to store the ISP\n>>> pointer in RkISP1CameraData. You can access the pipeline handler here:\n>>>\n>>>     PipelineHandlerRkISP1 *pipe =\n>>>         static_cast(data_->pipe_);\n>>>     V4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n>>>\n>>>> +    if (ispFormats.empty()) {\n>>>> +        LOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n>>>> +        return Invalid;\n>>>> +    }\n>>>\n>>> Missing blank line.\n>>>\n>>>> +    /*\n>>>> +     * The maximum resolution is identical for all media bus codes on\n>>>> +     * the RkISP1 isp entity. Therefore take the first available resolution.\n>>>> +     */\n>>>> +    Size ispMaximum = ispFormats.begin()->second[0].max;\n>>>> +\n>>>> +    /*\n>>>> +     * Select the sensor format, use either the best fit to the configured\n>>>> +     * format or a specific sensor format, when getFormat would choose a\n>>>> +     * resolution that surpasses the ISP maximum.\n>>>> +     */\n>>>> +    Size maxSensorSize;\n>>>> +    for (const Size &size : sensor->sizes()) {\n>>>> +        if (size.width > ispMaximum.width ||\n>>>> +            size.height > ispMaximum.height)\n>>>> +            continue;\n>>>\n>>> This makes me think we need better comparison functions for the Size\n>>> class. Maybe a Size::contains() function ? It doesn't have to be part of\n>>> this series.\n>>>\n>>>> +        maxSensorSize = std::max(maxSensorSize, size);\n>>>> +    }\n>>>\n>>> Missing blank line here too.\n>>>\n>>> Could we move all the code above to\n>>> PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n>>> RkISP1CameraData, to avoid doing the calculation every time validate()\n>>> is called ?\n>>>\n>>>\n>>>> +    Size maxConfigSize;\n>>>>      for (const StreamConfiguration &cfg : config_)\n>>>> -        maxSize = std::max(maxSize, cfg.size);\n>>>> +        maxConfigSize = std::max(maxConfigSize, cfg.size);\n>>>> +\n>>>> +    if (maxConfigSize.height <= maxSensorSize.height &&\n>>>> +        maxConfigSize.width <= maxSensorSize.width)\n>>>> +        maxSensorSize = maxConfigSize;\n>>>>      sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n>>\n>> I wonder if it won't be an easier solution to add another optional parameter\n>> to the getFormat method of the sensor that limits the size that the sensor\n>> should return. This might benefit other pipline-handlers as well where\n>> a sensor is connected to an entity that has a maximal size it can accept.\n>> In rkisp1 all the above code could be replaced by just adding\n>> Size(4032,3024) as another parameter to getFormat.\n>>\n>> Thanks,\n>> Dafna\n>>\n>>>>                          MEDIA_BUS_FMT_SGBRG12_1X12,\n>>>> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>>>                          MEDIA_BUS_FMT_SGBRG8_1X8,\n>>>>                          MEDIA_BUS_FMT_SGRBG8_1X8,\n>>>>                          MEDIA_BUS_FMT_SRGGB8_1X8 },\n>>>> -                      maxSize);\n>>>> +                      maxSensorSize);\n>>>>      if (sensorFormat_.size.isNull())\n>>>>          sensorFormat_.size = sensor->resolution();\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 AA046C32E4\n\tfor ;\n\tTue, 23 Mar 2021 11:56:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F37DF68D63;\n\tTue, 23 Mar 2021 12:56:17 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C4C5602D7\n\tfor ;\n\tTue, 23 Mar 2021 12:56:17 +0100 (CET)","from [IPv6:2a02:810a:880:f54:84b5:fd36:ff26:2fc6] (unknown\n\t[IPv6:2a02:810a:880:f54:84b5:fd36:ff26:2fc6])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: dafna)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id ECE441F44C6B;\n\tTue, 23 Mar 2021 11:56:16 +0000 (GMT)"],"To":"Sebastian Fricke ","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t\n\t<20210319071901.rbecfp5z6zxavnit@basti-TUXEDO-Book-XA1510>","From":"Dafna Hirschfeld ","Message-ID":"<820cec27-63c9-02c4-60a1-0ff9af9dd283@collabora.com>","Date":"Tue, 23 Mar 2021 12:56:11 +0100","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":"<20210319071901.rbecfp5z6zxavnit@basti-TUXEDO-Book-XA1510>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15844,"web_url":"https://patchwork.libcamera.org/comment/15844/","msgid":"","date":"2021-03-24T01:31:51","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sebastian and Dafna,\n\nSorry for the late reply.\n\nOn Wed, Mar 17, 2021 at 08:07:16AM +0100, Sebastian Fricke wrote:\n> Hello Dafna, Laurent and Heiko,\n> \n> thank you all for your reviews, I have looked further into the problem\n> and would like to share my thoughts with you.\n> \n> Comments below...\n> \n> On 15.03.2021 17:52, Dafna Hirschfeld wrote:\n> > On 12.03.21 22:31, Laurent Pinchart wrote:\n> >> On Tue, Mar 02, 2021 at 09:16:04AM +0100, Dafna Hirschfeld wrote:\n> >>> On 02.03.21 03:39, Laurent Pinchart wrote:\n> >>>> On Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:\n> >>>>> On 28.02.21 16:49, Laurent Pinchart wrote:\n> >>>>>> On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n> >>>>>>> This patch fixes a mismatch of image formats during the pipeline\n> >>>>>>> creation of the RkISP1. The mismatch happens because the current code\n> >>>>>>> does not check if the configured format exceeds the maximum viable\n> >>>>>>> resolution of the ISP.\n> >>>>>>> \n> >>>>>>> Make sure to use a sensor format resolution that is smaller or equal to\n> >>>>>>> the maximum allowed resolution for the RkISP1. The maximum resolution\n> >>>>>>> is defined within the `rkisp1-common.h` file as:\n> >>>>>>> define RKISP1_ISP_MAX_WIDTH 4032\n> >>>>>>> define RKISP1_ISP_MAX_HEIGHT 3024\n> >>>>>>> \n> >>>>>>> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n> >>>>>>> the configured resolution.\n> >>>>>>> \n> >>>>>>> This means that some camera sensors can never operate with their maximum\n> >>>>>>> resolution, for example on my OV13850 camera sensor, there are two\n> >>>>>>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n> >>>>>>> never be picked as it surpasses the maximum of the ISP.\n> >>>>>> \n> >>>>>> It would have been nice if the ISP had an input crop rectangle :-S It\n> >>>>>> would have allowed us to crop the input image a little bit to 4032x2992\n> >>>>>> (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n> >>>>>> double-checking, is there indeed no way to crop at the input, or could\n> >>>>>> it be that it's not implemented in the driver ? I can't find the\n> >>>>>> 4032x3024 limit in the documentation I have access to.\n> >>>>> \n> >>>>> The isp does support cropping on the sink pad.\n> >>>>> But currently the function v4l2_subdev_link_validate_default compares\n> >>>>> the dimensions defined in v4l2_subdev_format which are not the crop\n> >>>>> dimensions but the ones set by set_fmt. Is that wrong?\n> >>>> \n> >>>> I think so. If the ISP supports a larger size than 4032x3024 before\n> >>>> cropping, it should accept that on its sink pad, with the sink crop\n> >>>> rectangle being adjusted to never be larger than 4032x3024 (for instance\n> >>>> when userspace sets the format on the sink pad, the crop rectangle could\n> >>>> be automatically set to the same size, clamped to 4032x3024 and\n> >>>> centered). Userspace can then adjust the crop rectangle further if\n> >>>> necessary.\n> >>> \n> >>> In rkisp1-isp.c, there is diagram of the cropping regions.\n> >>> It says that the 4032x3024 limit is 'for black level'.\n> >>> Does that means that some sensors might send frames with black pixels in\n> >>> the edges that need to be cropped?\n> >> \n> >> In this context, I believe that black level refers to black level\n> >> correction (BLC in short), which is the process of subtracting a fixed\n> >> value from all pixels to account for leakage currents and other\n> >> parasitic effects in the sensor that makes fully black pixels have a\n> >> non-zero value. Sensors typically have a set of lines before the image\n> >> that is physically covered, and those lines can then be transmitted over\n> >> CSI-2. The average black level will then be computed on the SoC side\n> >> (either using the CPU, or in the ISP if it supports doing so), and\n> >> configured in the ISP to subtract it from all pixels. The black lines\n> >> are cropped out of what the ISP processes further down the pipeline.\n> > \n> > The number of black/invalid lines depends on the sensor right?\n\nThat's right.\n\n> > So userspace should adjust the cropping according to the sensor.\n\nIt depends a bit on the sensor. Most sensors will not by default send\nthe optical black lines, so the ISP just doesn't receive them, and\nthere's nothing to crop. We can usually instruct the sensor to send\nthose lines (V4L2 is missing an API to do so properly), in which case\nthey can be send as a fixed number of lines before and/or after the\nimage that we would then need to instruct the ISP to crop (or to handle\nin a special way, depending on the ISP). For CSI-2 sensors, the optical\nblack lines can be tagged with a different data type, and the CSI-2\nreceiver can then (if it supports this feature) capture them in a\nseparate buffer. There are thus lots of options, depending on the\nhardware capabilities.\n\nIn this specific case, I don't think the sensor sends us any optical\nblack lines (at least not with the main image data type).\n\n> > If so then why would we want to always clamp to 4032x3024 ?\n> > Or should it just be the initial value and userspace can then increase\n> > the crop size?\n\nAs I understand it, the ISP can't process images larger than 4032x3024\n(due to limitations of the processing blocks inside the ISP). It however\ncan receive large frames at its input and crop them before any further\nprocessing, so a sensor that would send a size larger than 4032x3024\ncould be supported.\n\nThis is based on the information I have so far, I haven't studied the\nTRM in depth, so details may not be correct, especially the exact limits\non the resolution (information from different documents seems to\ndisagree, based on this e-mail discussion).\n\n> >>> The TRM says \"Maximum input resolution of 4416x3312 pixels\"\n> >>> The datasheet then shows that the default values are 4096x3072 for both the\n> >>> input resolution (ISP_ACQ) and for the corp in the sink (ISP_OUT).\n> >>> \n> >>> So from the docs at least it sound possible to do as you suggested,\n> >>> limit the input to 4416x3312 and then always set a crop with maximum\n> >>> value 4032x3024\n> >> \n> >> Sounds like it's worth a try at least :-)\n> \n> Dafna advised me to try and set the MAX and MIN size simply to\n> 4416x3312, so I did and here are the resulting images.\n> \n> I captured one video with these settings:\n> `LIBCAMERA_LOG_LEVELS=0 cam --camera=1 --capture=50 --file=/tmp/out_video -s height=600,width=800,pixelformat=NV12,role=video`\n> \n> libcamera then sets the camera pipeline like this:\n> ```\n> [0:35:57.915030227] [6669] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 2112x1568-SBGGR10_1X10\n> [0:35:57.915069602] [6669] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 2112x1568-SBGGR10_1X10\n> [0:35:57.915125018] [6669] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568\n> [0:35:57.915152143] [6669] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568\n> [0:35:57.915183643] [6669] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568\n> [0:35:57.915234684] [6669] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568\n> [0:35:57.915262101] [6669] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 800x600-YUYV8_2X8\n> [0:35:57.915284850] [6669] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 800x600-YUYV8_1_5X8\n> ```\n> \n> And another video with these settings:\n> `cam --camera=1 --capture=50 --file=/tmp/out_video -s height=2160,width=3840,pixelformat=NV12,role=video`\n> \n> libcamera sets the camera pipeline like this:\n> ```\n> [0:52:25.445115742] [8997] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10\n> [0:52:25.445158617] [8997] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10\n> [0:52:25.445214616] [8997] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136\n> [0:52:25.445242616] [8997] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n> [0:52:25.445274991] [8997] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n> [0:52:25.445325449] [8997] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n> [0:52:25.445351116] [8997] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3840x2160-YUYV8_2X8\n> [0:52:25.445372990] [8997] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3840x2160-YUYV8_1_5X8\n> ```\n> \n> Here are the results of both captures (I have taken roughly the same\n> frame number):\n> https://imgur.com/a/EFtEmB9\n> \n> The image quality of the higher resolution image is obviously better and\n> it is also quite a lot brighter. I was not able to detect any defect\n> pixels in the image so far.\n> But I noticed the following:\n> My sensor has a Black Level Calibration (BLC) register (@ 0x5001),\n> usually, BLC is enabled. If I turn it off and try to capture with a\n> sensor resolution of 4224x3136, then the pipeline can still be\n> created, validated and the buffers are queued, but it seems like the sensor\n> doesn't start anymore. When I use the lower resolution mode of the sensor\n> (2112x1568), while deactivating BLC everything works just fine.\n> So, something happens when BLC is off when the resolution is greater\n> than 4032x3024. (Sadly no part of the camera pipeline actually throws an\n> error)\n\nThat sounds very fishy. BLC is the process of subtracting the black\nlevel from all pixels. It shouldn't affect anything that the rkisp1\nwould care about. It may be an issue internal to the sensor.\n\nThis being said, maybe there's a side effect of the sensor sending the\nblack lines out and thus making the image size larger when BLC is\ndisabled (unlikely, but OmniVision...). This could affect the ISP. You\ncould try to play with cropping on the sensor side to reduce the height\na little bit and see if it fixes things.\n\n> Here is the debug log from cam:\n> ```\n> [0:11:23.311130186] [4131] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10\n> [0:11:23.311269310] [4131] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10\n> [0:11:23.311451309] [4131] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136\n> [0:11:23.311571766] [4131] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n> [0:11:23.311728682] [4131] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n> [0:11:23.311918555] [4131] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136\n> [0:11:23.312134679] [4131] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8\n> [0:11:23.312276428] [4131] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 1920x1920-YUYV8_1_5X8\n> [0:11:23.313918792] [4131] INFO IPARkISP1 rkisp1.cpp:120 Exposure: 4-3324 Gain: 16-248\n> [0:11:23.314731370] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Analogue Gain to 16 at index 1\n> [0:11:23.314913660] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Exposure to 4 at index 1\n> [0:11:23.336376391] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.\n> [0:11:23.337981130] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 0 buffers requested.\n> [0:11:23.338467627] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n> [0:11:23.339042498] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n> [0:11:23.339212247] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n> [0:11:23.339364496] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0\n> [0:11:23.339509453] [4130] DEBUG Camera camera.cpp:1029 Starting capture\n> [0:11:23.340191657] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video3[15:out]: 4 buffers requested.\n> [0:11:23.341328566] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video2[14:cap]: 4 buffers requested.\n> [0:11:23.344065548] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.\n> [0:11:23.344281088] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1351 /dev/video0[17:cap]: Prepared to import 4 buffers\n> Capture 5 frames\n> [0:11:23.346221533] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 0\n> [0:11:23.346598364] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 0\n> [0:11:23.346800779] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 0\n> [0:11:23.347141443] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 1\n> [0:11:23.347319651] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 1\n> [0:11:23.347473941] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 1\n> [0:11:23.347759189] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 2\n> [0:11:23.347921938] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 2\n> [0:11:23.348139520] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 2\n> [0:11:23.404159310] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 3\n> [0:11:23.404329934] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 3\n> [0:11:23.404401100] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 3\n> [0:11:23.404646390] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video3[15:out]: Dequeuing buffer 0\n> ^CExiting\n> ```\n> \n> Conclusion:\n> At first glance, I thought it might be fine to just increase the\n> resolution limits, but after seeing that this only works when the sensor\n> performs black level calibration, I think that it would be unwise to\n> perform this change.\n\nCan't we always enable BLC on the sensor ?\n\n> I am now working on the initial idea to crop the sink pad of the ISP\n> down to 4032x3024, if and only if the input resolution is greater than\n> 4032x3024 and smaller or equal to 4416x3312 (because I don't want to\n> create a crop to 4032x3024 when the input resolution is for example\n> 2112x1568). That crop is automatically propagated to the source pad of\n> the ISP within `rkisp1_isp_set_sink_crop` and within\n> `rkisp1_isp_set_src_fmt` we use the crop resolution as format\n> resolution. I will post that patch soon to the mailing list.\n> \n> >>>>>>> Signed-off-by: Sebastian Fricke \n> >>>>>>> ---\n> >>>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n> >>>>>>> 1 file changed, 31 insertions(+), 4 deletions(-)\n> >>>>>>> \n> >>>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>>>>> index 50eaa6a4..56a406c1 100644\n> >>>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>>>>> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>>>>>> \t\treturn Invalid;\n> >>>>>>> \t}\n> >>>>>>> -\t/* Select the sensor format. */\n> >>>>>>> -\tSize maxSize;\n> >>>>>>> +\t/* Get the ISP resolution limits */\n> >>>>>>> +\tV4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n> >>>>>> \n> >>>>>> Related to 1/2, note that you don't necessarily need to store the ISP\n> >>>>>> pointer in RkISP1CameraData. You can access the pipeline handler here:\n> >>>>>> \n> >>>>>> \tPipelineHandlerRkISP1 *pipe =\n> >>>>>> \t\tstatic_cast(data_->pipe_);\n> >>>>>> \tV4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n> >>>>>> \n> >>>>>>> +\tif (ispFormats.empty()) {\n> >>>>>>> +\t\tLOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n> >>>>>>> +\t\treturn Invalid;\n> >>>>>>> +\t}\n> >>>>>> \n> >>>>>> Missing blank line.\n> >>>>>> \n> >>>>>>> +\t/*\n> >>>>>>> +\t * The maximum resolution is identical for all media bus codes on\n> >>>>>>> +\t * the RkISP1 isp entity. Therefore take the first available resolution.\n> >>>>>>> +\t */\n> >>>>>>> +\tSize ispMaximum = ispFormats.begin()->second[0].max;\n> >>>>>>> +\n> >>>>>>> +\t/*\n> >>>>>>> +\t * Select the sensor format, use either the best fit to the configured\n> >>>>>>> +\t * format or a specific sensor format, when getFormat would choose a\n> >>>>>>> +\t * resolution that surpasses the ISP maximum.\n> >>>>>>> +\t */\n> >>>>>>> +\tSize maxSensorSize;\n> >>>>>>> +\tfor (const Size &size : sensor->sizes()) {\n> >>>>>>> +\t\tif (size.width > ispMaximum.width ||\n> >>>>>>> +\t\t size.height > ispMaximum.height)\n> >>>>>>> +\t\t\tcontinue;\n> >>>>>> \n> >>>>>> This makes me think we need better comparison functions for the Size\n> >>>>>> class. Maybe a Size::contains() function ? It doesn't have to be part of\n> >>>>>> this series.\n> \n> I would love to work on that right after this patch and the connected\n> kernel patch :).\n> \n> >>>>>> \n> >>>>>>> +\t\tmaxSensorSize = std::max(maxSensorSize, size);\n> >>>>>>> +\t}\n> >>>>>> \n> >>>>>> Missing blank line here too.\n> >>>>>> \n> >>>>>> Could we move all the code above to\n> >>>>>> PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n> >>>>>> RkISP1CameraData, to avoid doing the calculation every time validate()\n> >>>>>> is called ?\n> \n> Yes, that is a good idea, and actually when I think about it the most\n> logical location. I hope that my use case will be handled by the kernel\n> patch, but I will still post this patch in case a camera sensor comes\n> along with a resolution greater than 4416x3312. I will test the\n> libcamera patch without the kernel patch to make sure that it works. But\n> it would be great if someone could test the patch in combination with\n> the kernel patch, with an appropriate sensor. (I would also be open to\n> any recommendations as I don't know if such a sensor exists for my\n> platform).\n> \n> >>>>>>> +\tSize maxConfigSize;\n> >>>>>>> \tfor (const StreamConfiguration &cfg : config_)\n> >>>>>>> -\t\tmaxSize = std::max(maxSize, cfg.size);\n> >>>>>>> +\t\tmaxConfigSize = std::max(maxConfigSize, cfg.size);\n> >>>>>>> +\n> >>>>>>> +\tif (maxConfigSize.height <= maxSensorSize.height &&\n> >>>>>>> +\t maxConfigSize.width <= maxSensorSize.width)\n> >>>>>>> +\t\tmaxSensorSize = maxConfigSize;\n> >>>>>>> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> >>>>> \n> >>>>> I wonder if it won't be an easier solution to add another optional parameter\n> >>>>> to the getFormat method of the sensor that limits the size that the sensor\n> >>>>> should return. This might benefit other pipline-handlers as well where\n> >>>>> a sensor is connected to an entity that has a maximal size it can accept.\n> >>>>> In rkisp1 all the above code could be replaced by just adding\n> >>>>> Size(4032,3024) as another parameter to getFormat.\n> \n> I could add that in another patch, but I believe that if I don't require\n> that for the rkisp1, I would first wait for an actual use case to\n> appear.\n> \n> >>>>> \n> >>>>>>> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> >>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> >>>>>>> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> >>>>>>> -\t\t\t\t\t maxSize);\n> >>>>>>> +\t\t\t\t\t maxSensorSize);\n> >>>>>>> \tif (sensorFormat_.size.isNull())\n> >>>>>>> \t\tsensorFormat_.size = sensor->resolution();","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 E710BC32E5\n\tfor ;\n\tWed, 24 Mar 2021 01:32:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17B9368D65;\n\tWed, 24 Mar 2021 02:32:37 +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 1997B6051A\n\tfor ;\n\tWed, 24 Mar 2021 02:32:35 +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 7478B580;\n\tWed, 24 Mar 2021 02:32:34 +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=\"foslJvDu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616549554;\n\tbh=wblsF+dhJEyBwIWgSIqGooX7xyWE1TrB72mPJKUWQDs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=foslJvDuuGN0wzTwgx2pQbrHD/DssgBxuMPD7sxNk5ugsQKl7REMpuKXmPUQa5Hrf\n\tObeIUPInpgIk84ky2GbLYBrAzLHpR+3ENHw3Oco/WXLYGyIQqgIBCpd10mfRftDYBf\n\tfOR3u8mEBUfcEyJmbb8uRbFHP7wn2xWM0Z6aIdM8=","Date":"Wed, 24 Mar 2021 03:31:51 +0200","From":"Laurent Pinchart ","To":"Sebastian Fricke ","Message-ID":"","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t\n\t\n\t<6274ed99-d62a-a995-3b79-4d0f76c533d6@collabora.com>\n\t\n\t<8f1b35b1-feb8-7e04-095a-d70c8db315d3@collabora.com>\n\t<20210317070716.mjhssi4je7irynqa@basti-TUXEDO-Book-XA1510>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210317070716.mjhssi4je7irynqa@basti-TUXEDO-Book-XA1510>","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15845,"web_url":"https://patchwork.libcamera.org/comment/15845/","msgid":"","date":"2021-03-24T01:41:39","subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna and Sebastian,\n\nOn Tue, Mar 23, 2021 at 12:56:11PM +0100, Dafna Hirschfeld wrote:\n> On 19.03.21 08:19, Sebastian Fricke wrote:\n> > On 01.03.2021 08:48, Dafna Hirschfeld wrote:\n> >> On 28.02.21 16:49, Laurent Pinchart wrote:\n> >>> On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:\n> >>>> This patch fixes a mismatch of image formats during the pipeline\n> >>>> creation of the RkISP1. The mismatch happens because the current code\n> >>>> does not check if the configured format exceeds the maximum viable\n> >>>> resolution of the ISP.\n> >>>>\n> >>>> Make sure to use a sensor format resolution that is smaller or equal to\n> >>>> the maximum allowed resolution for the RkISP1. The maximum resolution\n> >>>> is defined within the `rkisp1-common.h` file as:\n> >>>> define RKISP1_ISP_MAX_WIDTH 4032\n> >>>> define RKISP1_ISP_MAX_HEIGHT 3024\n> >>>>\n> >>>> Enumerate the frame-sizes of the ISP entity and compare the maximum with\n> >>>> the configured resolution.\n> >>>>\n> >>>> This means that some camera sensors can never operate with their maximum\n> >>>> resolution, for example on my OV13850 camera sensor, there are two\n> >>>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will\n> >>>> never be picked as it surpasses the maximum of the ISP.\n> >>>\n> >>> It would have been nice if the ISP had an input crop rectangle :-S It\n> >>> would have allowed us to crop the input image a little bit to 4032x2992\n> >>> (keeping the same aspect ratio) or 4032x3024 (4:3). Just\n> >>> double-checking, is there indeed no way to crop at the input, or could\n> >>> it be that it's not implemented in the driver ? I can't find the\n> >>> 4032x3024 limit in the documentation I have access to.\n> >>\n> >> The isp does support cropping on the sink pad.\n> >> But currently the function v4l2_subdev_link_validate_default compares\n> >> the dimensions defined in v4l2_subdev_format which are not the crop\n> >> dimensions but the ones set by set_fmt. Is that wrong?\n> > \n> > I have looked into the code and I noticed that the cropping ability of\n> > the ISP sink pad is more like a dummy functionality. You are able to\n> > set a crop that is smaller than the format for the sink pad, but there\n> > is a chain of events that make this condition invalid. When\n> > `rkisp1_isp_set_sink_fmt` is called it automatically calls\n> > `rkisp1_isp_set_sink_crop` with the last used crop for the pad\n> > (0,0,800,600) by default. Within `set_sink_crop` the crop is aligned to\n> > the `sink_fmt` so that it is within the bounds of the new format. Then\n> > it goes on to call `rkisp1_isp_set_src_crop` with the last used crop on\n> > the source pad, it maps the `src_crop` into the `sink_crop`. And finally\n> > `rkisp1_isp_set_src_fmt` is called, which sets the width and height of\n> > the format with the width and height of the crop.\n> > We validate the links with `v4l2_subdev_link_validate_default`, which\n> > checks if the formats of the pads are equal.\n> > \n> > Therefore I can state the following:\n> > - the sink pad crop is bounded by the sink pad format\n> > - the source pad crop is bounded by the sink pad crop\n> > - the source pad format is bounded by the source pad format\n> > Meaning: when the sink pad format != sink pad crop, then the sink pad format\n> > and source pad format can never be equal. And therefore can never be\n> > validated.\n> \n> Hi, note that the function v4l2_subdev_link_validate_default compares\n> the pads of a link, that is, the two pads belong to different entities\n> in both side of the link. So inside the isp entity it is valid to have\n> \"sink_fmt > sink_crop > src_fmt > src_crop\" since the src_* and sink_*\n> values belong to different pads of the same entity and not to pads of\n> the same link.\n\nIt's actually more than valid, it's fairly normal, given that cropping\ncan only produce a smaller (or equal) image :-)\n\n> > The rockchip documentation of the ISP1 driver (http://opensource.rock-chips.com/wiki_Rockchip-isp1#V4l2_view),\n> > mentions the following statement for the sink pad of the ISP:\n> > \"the size should be equal/less than sensor input size.\"\n> > and for the source pad of the ISP:\n> > \"The size should be equal/less than sink pad size.\"\n\nNote that there are two things to consider: the hardware capabilities,\nand the driver implementation. The latter could be incorrect in some\nareas (which is the topic of this whole e-mail thread), so you can\nchallenge any driver implementation decision if they don't match the\nhardware capabilities.\n\n> > This means from the ISP's point of view, the following pad configuration\n> > should probably work:\n> > ```\n> > - entity 1: rkisp1_isp (4 pads, 5 links)\n> >     ...\n> >     pad0: Sink\n> >         [fmt:SBGGR10_1X10/4224x3136 field:none\n> >          crop.bounds:(0,0)/4224x3136\n> >          crop:(96,72)/4032x2992]\n> >         <- \"ov13850 1-0010\":0 [ENABLED]\n> >     ...\n> >     pad2: Source\n> >         [fmt:YUYV8_2X8/4032x2992 field:none\n> >          crop.bounds:(0,0)/4032x2992\n> >          crop:(0,0)/4032x2992]\n> >         -> \"rkisp1_resizer_mainpath\":0 [ENABLED]\n> >         -> \"rkisp1_resizer_selfpath\":0 []\n> >     ...\n> > - entity 6: rkisp1_resizer_mainpath (2 pads, 2 links)\n> >     ...\n> >     pad0: Sink\n> >         [fmt:YUYV8_2X8/4032x2992 field:none\n> >          crop.bounds:(0,0)/4032x2992\n> >          crop:(0,0)/4032x2992]\n> >         <- \"rkisp1_isp\":2 [ENABLED]\n> >     pad1: Source\n> >         [fmt:YUYV8_2X8/1920x1920 field:none]\n> >         -> \"rkisp1_mainpath\":0 [ENABLED,IMMUTABLE]\n> > - entity 28: ov13850 1-0010 (1 pad, 1 link)\n> >     ...\n> >     pad0: Source\n> >         [fmt:SBGGR10_1X10/4224x3136@20000/150000 field:none\n> >          crop.bounds:(16,8)/4224x3136\n> >          crop:(16,8)/4224x3136]\n> >         -> \"rkisp1_isp\":0 [ENABLED]\n> \n> This configuration seems valid to me.\n> Do you get EPIPE when streaming with this configuration?\n\nThe configuration seems valid to me too.\n\n> > ```\n> > \n> > But the problem is that this not how the link validation currently works,\n> > when the format sizes are not equal `v4l2_subdev_link_validate_default`,\n> > will definitely fail.\n\nI think failures need to be investigated, because, as Dafna explained,\nlink validation will compare formats on the two end of each link, not\nformats on different pads of the same subdev. Validation of the\nconfiguration of a given subdev (for instance the formats and crop\nrectangles on the rkisp1_isp subdev) is something that is handled by the\nsubdev drivers themselves, at .set_format() and .set_selection() time,\nnot when starting the stream.\n\nYou can enable the debugging messages in\nv4l2_subdev_link_validate_default() to get more information about what\nfails exactly (and possibly add more debugging messages in other places\nif the existing ones don't provide enough information to debug the\nissue).\n\n> > Now, I have 3 ideas in mind for how to continue with this issue.\n> > 1. We could modify the callback to validate media links, that would\n> > explicitly handle ISP sink and source differently, and check if the crops\n> > align when the sink format size is greater than 4032x3024. I think that\n> > this option is quite ugly and too specific.\n> > 2. We could add a new function similar to `v4l2_subdev_link_validate_default`,\n> > that performs the same checks but additionally also checks if the crop\n> > of a sink pad is equal to the format of a source pad when the format\n> > size validation failed. I am not too sure about this either.\n> > 3. We don't touch this part of the code and I fix the issue only in\n> > libcamera and the 4224x3136 mode of my camera cannot be used :(.\n> > \n> > I am not too happy with any of the solutions, that I have proposed, I\n> > would love to hear your thoughts and ideas.\n> >\n> >>>> Signed-off-by: Sebastian Fricke \n> >>>> ---\n> >>>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---\n> >>>>  1 file changed, 31 insertions(+), 4 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> index 50eaa6a4..56a406c1 100644\n> >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>>>          return Invalid;\n> >>>>      }\n> >>>> -    /* Select the sensor format. */\n> >>>> -    Size maxSize;\n> >>>> +    /* Get the ISP resolution limits */\n> >>>> +    V4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);\n> >>>\n> >>> Related to 1/2, note that you don't necessarily need to store the ISP\n> >>> pointer in RkISP1CameraData. You can access the pipeline handler here:\n> >>>\n> >>>     PipelineHandlerRkISP1 *pipe =\n> >>>         static_cast(data_->pipe_);\n> >>>     V4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);\n> >>>\n> >>>> +    if (ispFormats.empty()) {\n> >>>> +        LOG(RkISP1, Error) << \"Unable to fetch ISP formats.\";\n> >>>> +        return Invalid;\n> >>>> +    }\n> >>>\n> >>> Missing blank line.\n> >>>\n> >>>> +    /*\n> >>>> +     * The maximum resolution is identical for all media bus codes on\n> >>>> +     * the RkISP1 isp entity. Therefore take the first available resolution.\n> >>>> +     */\n> >>>> +    Size ispMaximum = ispFormats.begin()->second[0].max;\n> >>>> +\n> >>>> +    /*\n> >>>> +     * Select the sensor format, use either the best fit to the configured\n> >>>> +     * format or a specific sensor format, when getFormat would choose a\n> >>>> +     * resolution that surpasses the ISP maximum.\n> >>>> +     */\n> >>>> +    Size maxSensorSize;\n> >>>> +    for (const Size &size : sensor->sizes()) {\n> >>>> +        if (size.width > ispMaximum.width ||\n> >>>> +            size.height > ispMaximum.height)\n> >>>> +            continue;\n> >>>\n> >>> This makes me think we need better comparison functions for the Size\n> >>> class. Maybe a Size::contains() function ? It doesn't have to be part of\n> >>> this series.\n> >>>\n> >>>> +        maxSensorSize = std::max(maxSensorSize, size);\n> >>>> +    }\n> >>>\n> >>> Missing blank line here too.\n> >>>\n> >>> Could we move all the code above to\n> >>> PipelineHandlerRkISP1::createCamera() and store maxSensorSize in\n> >>> RkISP1CameraData, to avoid doing the calculation every time validate()\n> >>> is called ?\n> >>>\n> >>>\n> >>>> +    Size maxConfigSize;\n> >>>>      for (const StreamConfiguration &cfg : config_)\n> >>>> -        maxSize = std::max(maxSize, cfg.size);\n> >>>> +        maxConfigSize = std::max(maxConfigSize, cfg.size);\n> >>>> +\n> >>>> +    if (maxConfigSize.height <= maxSensorSize.height &&\n> >>>> +        maxConfigSize.width <= maxSensorSize.width)\n> >>>> +        maxSensorSize = maxConfigSize;\n> >>>>      sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> >>\n> >> I wonder if it won't be an easier solution to add another optional parameter\n> >> to the getFormat method of the sensor that limits the size that the sensor\n> >> should return. This might benefit other pipline-handlers as well where\n> >> a sensor is connected to an entity that has a maximal size it can accept.\n> >> In rkisp1 all the above code could be replaced by just adding\n> >> Size(4032,3024) as another parameter to getFormat.\n> >>\n> >> Thanks,\n> >> Dafna\n> >>\n> >>>>                          MEDIA_BUS_FMT_SGBRG12_1X12,\n> >>>> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>>>                          MEDIA_BUS_FMT_SGBRG8_1X8,\n> >>>>                          MEDIA_BUS_FMT_SGRBG8_1X8,\n> >>>>                          MEDIA_BUS_FMT_SRGGB8_1X8 },\n> >>>> -                      maxSize);\n> >>>> +                      maxSensorSize);\n> >>>>      if (sensorFormat_.size.isNull())\n> >>>>          sensorFormat_.size = sensor->resolution();\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 B733BC32E4\n\tfor ;\n\tWed, 24 Mar 2021 01:42:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0461C68D62;\n\tWed, 24 Mar 2021 02:42:24 +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 C23956051A\n\tfor ;\n\tWed, 24 Mar 2021 02:42:22 +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 2B699580;\n\tWed, 24 Mar 2021 02:42:22 +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=\"RUbrjUJL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616550142;\n\tbh=Ts8Lv8QOcqnDqCUSoguTNvzYkl1wd+cG67vy/g44O1o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RUbrjUJLtOrtK7uHM6CxqOoo3aWx8Atc1jpGastbjAJJHtH+lmISkyTUHN56No+Ec\n\tKn3VOJtHeSkV3+l39hNPzfFEy/SNCbVjKQXhAb4GSDVV8DmJlWqjtL7++F9E9GJY1h\n\t0ASnq0gnrWGS7kbNsix214ufP3c0SOU7agN6UML8=","Date":"Wed, 24 Mar 2021 03:41:39 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210227180126.37591-1-sebastian.fricke@posteo.net>\n\t<20210227180126.37591-3-sebastian.fricke@posteo.net>\n\t\n\t\n\t<20210319071901.rbecfp5z6zxavnit@basti-TUXEDO-Book-XA1510>\n\t<820cec27-63c9-02c4-60a1-0ff9af9dd283@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<820cec27-63c9-02c4-60a1-0ff9af9dd283@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor\n\tISP format mismatch","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,\n\tCollabora Kernel ML ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]