[{"id":15580,"web_url":"https://patchwork.libcamera.org/comment/15580/","msgid":"","date":"2021-03-09T22:39:34","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: rkisp1: fail on init if\n\thw revision is not RKISP1_V10","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nThank you for the patch.\n\nOn Tue, Mar 09, 2021 at 07:38:29AM +0100, Dafna Hirschfeld wrote:\n> In kernel 5.11 the rkisp1 uapi had changed to support\n> different hardware revisions. Currently only revision 10\n> is supported by the rkisp1 IPA and therefore 'init'\n> should fail if the revision is not 10.\n\nI think it's important here to state that this change will require a\nkernel upgrade to v5.11 or newer. Maybe as follow ?\n\nThis changes depends on the kernel driver reporting the hardware\nrevision, and thus requires the rkisp1 driver from v5.11 or newer.\n\n> Signed-off-by: Dafna Hirschfeld \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++++++++----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++------\n> 3 files changed, 18 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index 95fa0d93..29f726e1 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -25,7 +25,7 @@ struct RkISP1Action {\n> };\n> \n> interface IPARkISP1Interface {\n> -\tinit(IPASettings settings) => (int32 ret);\n> +\tinit(uint32 hwRevision) => (int32 ret);\n> \tstart() => (int32 ret);\n> \tstop();\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 0b0f31e4..197c2389 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -31,10 +31,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)\n> class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface\n> {\n> public:\n> -\tint init([[maybe_unused]] const IPASettings &settings) override\n> -\t{\n> -\t\treturn 0;\n> -\t}\n> +\tint init(unsigned int hwRevision) override;\n\nI expect we'll need IPA settings in the future, but that's fine, we can\nadd it back later when/if needed.\n\n> \tint start() override { return 0; }\n> \tvoid stop() override {}\n> \n> @@ -69,6 +66,18 @@ private:\n> \tuint32_t maxGain_;\n> };\n> \n> +int IPARkISP1::init(unsigned int hwRevision)\n> +{\n> +\t/* todo add support for other revisions */\n\nThis should be \\todo.\n\n> +\tif (hwRevision != RKISP1_V10) {\n> +\t\tLOG(IPARkISP1, Error) << \"Hardware version \" << hwRevision <<\n\ns/version/revision/ ?\n\n> +\t\t\t\" is currently not supported\";\n\nHere's how we usually format multi-line log statements:\n\n\t\tLOG(IPARkISP1, Error)\n\t\t\t<< \"Hardware version \" << hwRevision\n\t\t\t<< \" is currently not supported\";\n\n> +\t\treturn -ENODEV;\n> +\t}\n> +\tLOG(IPARkISP1, Info) << \"Hardware revision is \" << hwRevision;\n\nI'd turn this into a debug message, as we support a single revision,\nit's not very important information at this point.\n\n> +\treturn 0;\n> +}\n> +;\n\nNo need for a semicolon here.\n\n> /**\n> * \\todo The RkISP1 pipeline currently provides an empty CameraSensorInfo\n> * if the connected sensor does not provide enough information to properly\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 34814f62..24c622a8 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -85,7 +85,7 @@ public:\n> \t{\n> \t}\n> \n> -\tint loadIPA();\n> +\tint loadIPA(unsigned int hwRevision);\n> \n> \tStream mainPathStream_;\n> \tStream selfPathStream_;\n> @@ -300,7 +300,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> \treturn nullptr;\n> }\n> \n> -int RkISP1CameraData::loadIPA()\n> +int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> {\n> \tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> \tif (!ipa_)\n> @@ -309,9 +309,7 @@ int RkISP1CameraData::loadIPA()\n> \tipa_->queueFrameAction.connect(this,\n> \t\t\t\t &RkISP1CameraData::queueFrameAction);\n> \n> -\tipa_->init(IPASettings{});\n> -\n> -\treturn 0;\n> +\treturn ipa_->init(hwRevision);\n\nHow about adding an error message here ?\n\n\tint ret = ipa_->init(hwRevision);\n\tif (ret < 0) {\n\t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n\t\treturn ret;\n\t}\n\n\treturn 0;\n\nOtherwise the pipeline handler will fail silently, and it may be hard to\ndebug the problem.\n\n> }\n> \n> void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> @@ -952,7 +950,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tisp_->frameStart.connect(data->delayedCtrls_.get(),\n> \t\t\t\t &DelayedControls::applyControls);\n> \n> -\tret = data->loadIPA();\n> +\tret = data->loadIPA(media_->hwRevision());\n> \tif (ret)\n> \t\treturn ret;\n> \n\nI've thought a bit more about version checks, and I'd propose adding\n\n\tif (!media_->hwRevision()) {\n\t\tLOG(RkISP1, Error)\n\t\t\t<< \"The rkisp1 driver is too old, v5.11 or newer is required\";\n\t\treturn false;\n\t}\n\nto the PipelineHandlerRkISP1::match() function. Otherwise, if the rkisp1\ndriver is too old (and v5.11 being quite recent, we will have developers\nrunning into this problem), the error messages that will be printed\nwon't give a clear indication of how to solve the issue. What do you\nthink ?\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\nIf you agree with all the comments, you can just let me know, and I'll\nmake changes when applying, there's no need to resubmit.","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 4AA1ABD1F1\n\tfor ;\n\tTue, 9 Mar 2021 22:40:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B61AF68AA3;\n\tTue, 9 Mar 2021 23:40:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7952168A99\n\tfor ;\n\tTue, 9 Mar 2021 23:40:07 +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 29577E9;\n\tTue, 9 Mar 2021 23:40:06 +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=\"fT93BWN3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615329606;\n\tbh=dFgpOooNk8l4c4UtmDCqBai//UUybTXb/0xsNITU3Fk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fT93BWN3YupnDRZKiu18yr7HwOBQVONDegjJx/SGb4UoZNxj0pQiqVjV6Na8wgSOD\n\tbJ64wOBcJ8+tQTmSL5svqr3jbq1y6+CPN2Umw2HZbxJ57ZrIEMbP96FMx2Mc0nwrse\n\tIkE+PlZXgh45JaMnL0QWiXqF44l5C+bjCXaMtDhM=","Date":"Wed, 10 Mar 2021 00:39:34 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210309063829.8710-1-dafna.hirschfeld@collabora.com>\n\t<20210309063829.8710-4-dafna.hirschfeld@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210309063829.8710-4-dafna.hirschfeld@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: rkisp1: fail on init if\n\thw revision is not RKISP1_V10","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, kernel@collabora.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15582,"web_url":"https://patchwork.libcamera.org/comment/15582/","msgid":"<04fd3f15-1996-076a-f555-e62e230ccca5@collabora.com>","date":"2021-03-10T06:47:49","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: rkisp1: fail on init if\n\thw revision is not RKISP1_V10","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"Hi,\n\nOn 09.03.21 23:39, Laurent Pinchart wrote:\n> Hi Dafna,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 09, 2021 at 07:38:29AM +0100, Dafna Hirschfeld wrote:\n>> In kernel 5.11 the rkisp1 uapi had changed to support\n>> different hardware revisions. Currently only revision 10\n>> is supported by the rkisp1 IPA and therefore 'init'\n>> should fail if the revision is not 10.\n> \n> I think it's important here to state that this change will require a\n> kernel upgrade to v5.11 or newer. Maybe as follow ?\n> \n> This changes depends on the kernel driver reporting the hardware\n> revision, and thus requires the rkisp1 driver from v5.11 or newer.\n> \n>> Signed-off-by: Dafna Hirschfeld \n>> ---\n>> include/libcamera/ipa/rkisp1.mojom | 2 +-\n>> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++++++++----\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++------\n>> 3 files changed, 18 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n>> index 95fa0d93..29f726e1 100644\n>> --- a/include/libcamera/ipa/rkisp1.mojom\n>> +++ b/include/libcamera/ipa/rkisp1.mojom\n>> @@ -25,7 +25,7 @@ struct RkISP1Action {\n>> };\n>> \n>> interface IPARkISP1Interface {\n>> -\tinit(IPASettings settings) => (int32 ret);\n>> +\tinit(uint32 hwRevision) => (int32 ret);\n>> \tstart() => (int32 ret);\n>> \tstop();\n>> \n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 0b0f31e4..197c2389 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -31,10 +31,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)\n>> class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface\n>> {\n>> public:\n>> -\tint init([[maybe_unused]] const IPASettings &settings) override\n>> -\t{\n>> -\t\treturn 0;\n>> -\t}\n>> +\tint init(unsigned int hwRevision) override;\n> \n> I expect we'll need IPA settings in the future, but that's fine, we can\n> add it back later when/if needed.\n> \n>> \tint start() override { return 0; }\n>> \tvoid stop() override {}\n>> \n>> @@ -69,6 +66,18 @@ private:\n>> \tuint32_t maxGain_;\n>> };\n>> \n>> +int IPARkISP1::init(unsigned int hwRevision)\n>> +{\n>> +\t/* todo add support for other revisions */\n> \n> This should be \\todo.\n> \n>> +\tif (hwRevision != RKISP1_V10) {\n>> +\t\tLOG(IPARkISP1, Error) << \"Hardware version \" << hwRevision <<\n> \n> s/version/revision/ ?\n> \n>> +\t\t\t\" is currently not supported\";\n> \n> Here's how we usually format multi-line log statements:\n> \n> \t\tLOG(IPARkISP1, Error)\n> \t\t\t<< \"Hardware version \" << hwRevision\n> \t\t\t<< \" is currently not supported\";\n> \n>> +\t\treturn -ENODEV;\n>> +\t}\n>> +\tLOG(IPARkISP1, Info) << \"Hardware revision is \" << hwRevision;\n> \n> I'd turn this into a debug message, as we support a single revision,\n> it's not very important information at this point.\n> \n>> +\treturn 0;\n>> +}\n>> +;\n> \n> No need for a semicolon here.\n> \n>> /**\n>> * \\todo The RkISP1 pipeline currently provides an empty CameraSensorInfo\n>> * if the connected sensor does not provide enough information to properly\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index 34814f62..24c622a8 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -85,7 +85,7 @@ public:\n>> \t{\n>> \t}\n>> \n>> -\tint loadIPA();\n>> +\tint loadIPA(unsigned int hwRevision);\n>> \n>> \tStream mainPathStream_;\n>> \tStream selfPathStream_;\n>> @@ -300,7 +300,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n>> \treturn nullptr;\n>> }\n>> \n>> -int RkISP1CameraData::loadIPA()\n>> +int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n>> {\n>> \tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n>> \tif (!ipa_)\n>> @@ -309,9 +309,7 @@ int RkISP1CameraData::loadIPA()\n>> \tipa_->queueFrameAction.connect(this,\n>> \t\t\t\t &RkISP1CameraData::queueFrameAction);\n>> \n>> -\tipa_->init(IPASettings{});\n>> -\n>> -\treturn 0;\n>> +\treturn ipa_->init(hwRevision);\n> \n> How about adding an error message here ?\n> \n> \tint ret = ipa_->init(hwRevision);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> \t}\n> \n> \treturn 0;\n> \n> Otherwise the pipeline handler will fail silently, and it may be hard to\n> debug the problem.\n> \n>> }\n>> \n>> void RkISP1CameraData::queueFrameAction(unsigned int frame,\n>> @@ -952,7 +950,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>> \tisp_->frameStart.connect(data->delayedCtrls_.get(),\n>> \t\t\t\t &DelayedControls::applyControls);\n>> \n>> -\tret = data->loadIPA();\n>> +\tret = data->loadIPA(media_->hwRevision());\n>> \tif (ret)\n>> \t\treturn ret;\n>> \n> \n> I've thought a bit more about version checks, and I'd propose adding\n> \n> \tif (!media_->hwRevision()) {\n> \t\tLOG(RkISP1, Error)\n> \t\t\t<< \"The rkisp1 driver is too old, v5.11 or newer is required\";\n> \t\treturn false;\n> \t}\n> \n> to the PipelineHandlerRkISP1::match() function. Otherwise, if the rkisp1\n> driver is too old (and v5.11 being quite recent, we will have developers\n> running into this problem), the error messages that will be printed\n> won't give a clear indication of how to solve the issue. What do you\n> think ?\n\nI don't mind adding this check. The rkisp1 was a in staging until\nv5.11 so probably didn't have much users.\n\n> \n> With these small issues addressed,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> If you agree with all the comments, you can just let me know, and I'll\n> make changes when applying, there's no need to resubmit.\n\nokay, I agree to the comments, thank:), you mean the whole patchset?\n\nThanks,\nDafna\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 89E5EBD80C\n\tfor ;\n\tWed, 10 Mar 2021 06:47:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 10E6A68AA1;\n\tWed, 10 Mar 2021 07:47:55 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C9B9602E3\n\tfor ;\n\tWed, 10 Mar 2021 07:47:53 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: dafna) with ESMTPSA id DF8471F4588C"],"To":"Laurent Pinchart ","References":"<20210309063829.8710-1-dafna.hirschfeld@collabora.com>\n\t<20210309063829.8710-4-dafna.hirschfeld@collabora.com>\n\t","From":"Dafna Hirschfeld ","Message-ID":"<04fd3f15-1996-076a-f555-e62e230ccca5@collabora.com>","Date":"Wed, 10 Mar 2021 07:47:49 +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 v3 3/3] ipa: rkisp1: fail on init if\n\thw revision is not RKISP1_V10","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, kernel@collabora.com","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":15584,"web_url":"https://patchwork.libcamera.org/comment/15584/","msgid":"","date":"2021-03-10T08:32:39","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: rkisp1: fail on init if\n\thw revision is not RKISP1_V10","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nOn Wed, Mar 10, 2021 at 07:47:49AM +0100, Dafna Hirschfeld wrote:\n> On 09.03.21 23:39, Laurent Pinchart wrote:\n> > On Tue, Mar 09, 2021 at 07:38:29AM +0100, Dafna Hirschfeld wrote:\n> >> In kernel 5.11 the rkisp1 uapi had changed to support\n> >> different hardware revisions. Currently only revision 10\n> >> is supported by the rkisp1 IPA and therefore 'init'\n> >> should fail if the revision is not 10.\n> > \n> > I think it's important here to state that this change will require a\n> > kernel upgrade to v5.11 or newer. Maybe as follow ?\n> > \n> > This changes depends on the kernel driver reporting the hardware\n> > revision, and thus requires the rkisp1 driver from v5.11 or newer.\n> > \n> >> Signed-off-by: Dafna Hirschfeld \n> >> ---\n> >> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> >> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++++++++----\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++------\n> >> 3 files changed, 18 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> >> index 95fa0d93..29f726e1 100644\n> >> --- a/include/libcamera/ipa/rkisp1.mojom\n> >> +++ b/include/libcamera/ipa/rkisp1.mojom\n> >> @@ -25,7 +25,7 @@ struct RkISP1Action {\n> >> };\n> >> \n> >> interface IPARkISP1Interface {\n> >> -\tinit(IPASettings settings) => (int32 ret);\n> >> +\tinit(uint32 hwRevision) => (int32 ret);\n> >> \tstart() => (int32 ret);\n> >> \tstop();\n> >> \n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index 0b0f31e4..197c2389 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -31,10 +31,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)\n> >> class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface\n> >> {\n> >> public:\n> >> -\tint init([[maybe_unused]] const IPASettings &settings) override\n> >> -\t{\n> >> -\t\treturn 0;\n> >> -\t}\n> >> +\tint init(unsigned int hwRevision) override;\n> > \n> > I expect we'll need IPA settings in the future, but that's fine, we can\n> > add it back later when/if needed.\n> > \n> >> \tint start() override { return 0; }\n> >> \tvoid stop() override {}\n> >> \n> >> @@ -69,6 +66,18 @@ private:\n> >> \tuint32_t maxGain_;\n> >> };\n> >> \n> >> +int IPARkISP1::init(unsigned int hwRevision)\n> >> +{\n> >> +\t/* todo add support for other revisions */\n> > \n> > This should be \\todo.\n> > \n> >> +\tif (hwRevision != RKISP1_V10) {\n> >> +\t\tLOG(IPARkISP1, Error) << \"Hardware version \" << hwRevision <<\n> > \n> > s/version/revision/ ?\n> > \n> >> +\t\t\t\" is currently not supported\";\n> > \n> > Here's how we usually format multi-line log statements:\n> > \n> > \t\tLOG(IPARkISP1, Error)\n> > \t\t\t<< \"Hardware version \" << hwRevision\n> > \t\t\t<< \" is currently not supported\";\n> > \n> >> +\t\treturn -ENODEV;\n> >> +\t}\n> >> +\tLOG(IPARkISP1, Info) << \"Hardware revision is \" << hwRevision;\n> > \n> > I'd turn this into a debug message, as we support a single revision,\n> > it's not very important information at this point.\n> > \n> >> +\treturn 0;\n> >> +}\n> >> +;\n> > \n> > No need for a semicolon here.\n> > \n> >> /**\n> >> * \\todo The RkISP1 pipeline currently provides an empty CameraSensorInfo\n> >> * if the connected sensor does not provide enough information to properly\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index 34814f62..24c622a8 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -85,7 +85,7 @@ public:\n> >> \t{\n> >> \t}\n> >> \n> >> -\tint loadIPA();\n> >> +\tint loadIPA(unsigned int hwRevision);\n> >> \n> >> \tStream mainPathStream_;\n> >> \tStream selfPathStream_;\n> >> @@ -300,7 +300,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> >> \treturn nullptr;\n> >> }\n> >> \n> >> -int RkISP1CameraData::loadIPA()\n> >> +int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> >> {\n> >> \tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> >> \tif (!ipa_)\n> >> @@ -309,9 +309,7 @@ int RkISP1CameraData::loadIPA()\n> >> \tipa_->queueFrameAction.connect(this,\n> >> \t\t\t\t &RkISP1CameraData::queueFrameAction);\n> >> \n> >> -\tipa_->init(IPASettings{});\n> >> -\n> >> -\treturn 0;\n> >> +\treturn ipa_->init(hwRevision);\n> > \n> > How about adding an error message here ?\n> > \n> > \tint ret = ipa_->init(hwRevision);\n> > \tif (ret < 0) {\n> > \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> > \t\treturn ret;\n> > \t}\n> > \n> > \treturn 0;\n> > \n> > Otherwise the pipeline handler will fail silently, and it may be hard to\n> > debug the problem.\n> > \n> >> }\n> >> \n> >> void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> >> @@ -952,7 +950,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >> \tisp_->frameStart.connect(data->delayedCtrls_.get(),\n> >> \t\t\t\t &DelayedControls::applyControls);\n> >> \n> >> -\tret = data->loadIPA();\n> >> +\tret = data->loadIPA(media_->hwRevision());\n> >> \tif (ret)\n> >> \t\treturn ret;\n> >> \n> > \n> > I've thought a bit more about version checks, and I'd propose adding\n> > \n> > \tif (!media_->hwRevision()) {\n> > \t\tLOG(RkISP1, Error)\n> > \t\t\t<< \"The rkisp1 driver is too old, v5.11 or newer is required\";\n> > \t\treturn false;\n> > \t}\n> > \n> > to the PipelineHandlerRkISP1::match() function. Otherwise, if the rkisp1\n> > driver is too old (and v5.11 being quite recent, we will have developers\n> > running into this problem), the error messages that will be printed\n> > won't give a clear indication of how to solve the issue. What do you\n> > think ?\n> \n> I don't mind adding this check. The rkisp1 was a in staging until\n> v5.11 so probably didn't have much users.\n\nThat's true, but even if there are few users, Kieran would scold me for\nnot taking good care of them :-)\n\n> > With these small issues addressed,\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > If you agree with all the comments, you can just let me know, and I'll\n> > make changes when applying, there's no need to resubmit.\n> \n> okay, I agree to the comments, thank:), you mean the whole patchset?\n\nYes. I'll test it today, and then apply.","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 3DEB5BD1F1\n\tfor ;\n\tWed, 10 Mar 2021 08:33:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7B8168AA2;\n\tWed, 10 Mar 2021 09:33:13 +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 41033602ED\n\tfor ;\n\tWed, 10 Mar 2021 09:33:12 +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 9D171F3;\n\tWed, 10 Mar 2021 09:33:11 +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=\"ZNI3B4Vl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615365191;\n\tbh=PFiLJ3/ywCOTw22fZIf7AxjGeP7/EK4XmBhrPz7Plew=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZNI3B4VlyFOG13bbXsV6Fh/rSYqG3/mZPI+GuGAa7KUHp+tO8GepxrETimf3LoHDl\n\tMAkmGforc3F/uWtJo/rkrhpE/ZEVyS74ywiI0C+PyozYjsHm8u8DU6JY06zFdfZxKR\n\tP+smis8eHX40ytt5PqOoua3dakLtGGyh97zLLaE8=","Date":"Wed, 10 Mar 2021 10:32:39 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210309063829.8710-1-dafna.hirschfeld@collabora.com>\n\t<20210309063829.8710-4-dafna.hirschfeld@collabora.com>\n\t\n\t<04fd3f15-1996-076a-f555-e62e230ccca5@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<04fd3f15-1996-076a-f555-e62e230ccca5@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: rkisp1: fail on init if\n\thw revision is not RKISP1_V10","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, kernel@collabora.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]