[{"id":29370,"web_url":"https://patchwork.libcamera.org/comment/29370/","msgid":"","date":"2024-05-01T09:37:00","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch.\n\nOn Fri, 26 Apr 2024 at 12:18, Naushir Patuck wrote:\n>\n> The maximum shutter speed calculation in the cam-helper relied on\n> the frame duration limits being correctly set in the cam-helper's mode\n> structure. This was not the case on first startup, so the maximum\n> shutter speed reported back via the ControlInfo was incorrect.\n>\n> Fix this by setting up the camera mode in the cam-helper before querying\n> for the max shutter value.\n>\n> Signed-off-by: Naushir Patuck \n\nYes, looks sensible to me.\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 149a133ab662..1d12262bda01 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n>\n> + /*\n> + * We need to give the helper the min/max frame durations so it can calculate\n> + * the correct exposure limits below.\n> + */\n> + helper_->setCameraMode(mode_);\n> +\n> /* Shutter speed is calculated based on the limits of the frame durations. */\n> mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> mode_.maxShutter = Duration::max();\n> --\n> 2.34.1\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 502EABDE6B\n\tfor ;\n\tWed, 1 May 2024 09:37:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 419B86340B;\n\tWed, 1 May 2024 11:37:14 +0200 (CEST)","from mail-qk1-x72a.google.com (mail-qk1-x72a.google.com\n\t[IPv6:2607:f8b0:4864:20::72a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D35B61A8A\n\tfor ;\n\tWed, 1 May 2024 11:37:12 +0200 (CEST)","by mail-qk1-x72a.google.com with SMTP id\n\taf79cd13be357-78f04924a96so555122785a.0\n\tfor ;\n\tWed, 01 May 2024 02:37:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"qIU1ZOM+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1714556231; x=1715161031;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=1IytHyYCTn1jIWs2RbT8UZCTUIG01W6jt7LzYSigLsI=;\n\tb=qIU1ZOM+jzhziH6MTUASj07o9vXFTn0jCRys5kw4XSM5s/yUsj3CH1e4aBDSKi0eVY\n\t+F9uq03Ep6xQe7ia/MFI0Q5ywair6Pkn4Aac1Ymd7fOAjy0WP7eQWUL23jI4oYZqMI28\n\tsN0WnQXBVCYGeH8DyoXMMndXDOIQYUdqWZ1uqB+G/iUPrjutKJyfwvCQsJsJmqjwRRlf\n\tNQVXqTZ4uqGRInOpH2a6Gy1WkV3X1I1uxmpo7IWOP0pgiNVVxB8hFFkOkqm2dQokCfLK\n\tyGnmt3m1z2IKnaxFBI41QnTF6GGvkzErmC14/ZGdIrfSa1S5NaHGw4VgixdkVcXZ6YFp\n\t5UlQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1714556231; x=1715161031;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=1IytHyYCTn1jIWs2RbT8UZCTUIG01W6jt7LzYSigLsI=;\n\tb=KLPAhTlAcoKYBwO8np5XAwWBi13b6Rgmjmn8Im0F7APOUc0XNZuZb9UaS/J+LDU63t\n\t30JPMNbHSofCo2x77pzq2n8rIhlJwvsMKaMlg/Ux9m0J4dmQS8NDXbAlqvB1/anEw5X9\n\tZFgt3kQg1hDFUH/N8RBqfv/peRZbW+MbBFpXgEedQWCV9euUYpSV9lk4WNCIqEwncTA8\n\tXTwVWLab6U3VpwdGGm+SnlX+nEpASrfbAq1K0qia8+Qis3m1dFhxGRvzBSvDl2+l+lIm\n\t+c6SFJzYEevggOZzZ5fEsPIVWetsbq/rDmP4wncI1LsnP4CauFIDHewSworh5VOUGEGf\n\t1cZQ==","X-Gm-Message-State":"AOJu0YxjVivMnDwFoqThwjhoHL6t6y+WPt1HzY8s2bg+7WtP3w4JhlaH\n\thdtB+l9TR5QaTN5JXls+bKwcUOaOkuEq67cIb43gXe5eJHhwgTaK7wKQt2nky5pM0zmf8Vb/qzM\n\t4m/udoC0UXcVzLZlRoRNu4UFvrc+k6w/gEeOyxw==","X-Google-Smtp-Source":"AGHT+IFKQNa6X/7Xk84L0kkXnY37FR/aVr4Tu41I683n+49NK04DRgqh5MtYTchXbEeAli/F1ighEuT5urtqMECofvY=","X-Received":"by 2002:a05:6214:d6c:b0:6a0:caba:7e2f with SMTP id\n\t12-20020a0562140d6c00b006a0caba7e2fmr2059320qvs.20.1714556231298;\n\tWed, 01 May 2024 02:37:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20240426111815.10763-1-naush@raspberrypi.com>","In-Reply-To":"<20240426111815.10763-1-naush@raspberrypi.com>","From":"David Plowman ","Date":"Wed, 1 May 2024 10:37:00 +0100","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29376,"web_url":"https://patchwork.libcamera.org/comment/29376/","msgid":"<20240501155309.GD9891@pendragon.ideasonboard.com>","date":"2024-05-01T15:53:09","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> The maximum shutter speed calculation in the cam-helper relied on\n> the frame duration limits being correctly set in the cam-helper's mode\n> structure. This was not the case on first startup, so the maximum\n> shutter speed reported back via the ControlInfo was incorrect.\n> \n> Fix this by setting up the camera mode in the cam-helper before querying\n> for the max shutter value.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> 1 file changed, 6 insertions(+)\n> \n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 149a133ab662..1d12262bda01 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> \tmode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> \tmode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> \n> +\t/*\n> +\t * We need to give the helper the min/max frame durations so it can calculate\n> +\t * the correct exposure limits below.\n> +\t */\n> +\thelper_->setCameraMode(mode_);\n> +\n\nDon't you end up doing this twice, once here, and once in\nIpaBase::configure(), which calls IpaBase::setMode() ?\n\n> \t/* Shutter speed is calculated based on the limits of the frame durations. */\n> \tmode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> \tmode_.maxShutter = Duration::max();","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 CC781BDE6B\n\tfor ;\n\tWed, 1 May 2024 15:53:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE3FA63418;\n\tWed, 1 May 2024 17:53:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67A87633ED\n\tfor ;\n\tWed, 1 May 2024 17:53:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(233.56-78-194.adsl-static.isp.belgacom.be [194.78.56.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5971C21D;\n\tWed, 1 May 2024 17:52:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"r1+B0vYS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714578738;\n\tbh=KRJbo81+fBzNS2UkZrhX+LVHfN8YskBK6c6LGNWLU24=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r1+B0vYS3chQbakTHfAGKSZp9Ikalil0pXB6mnIpujEYY/fFrIvw83sBlpiIpTcK2\n\tOVKwXrzzCiVn/wnlsJT+PJHL1L6IHQvvGeoPc5Mr81HgLq3C8XpmL9Oe/qEKvU12r8\n\tptDdxAb3r7avNNs7Ap/h4CylvY2IjO+1gRUhIvVM=","Date":"Wed, 1 May 2024 18:53:09 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","Message-ID":"<20240501155309.GD9891@pendragon.ideasonboard.com>","References":"<20240426111815.10763-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240426111815.10763-1-naush@raspberrypi.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29377,"web_url":"https://patchwork.libcamera.org/comment/29377/","msgid":"","date":"2024-05-01T16:04:03","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 1 May 2024 at 16:53, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > The maximum shutter speed calculation in the cam-helper relied on\n> > the frame duration limits being correctly set in the cam-helper's mode\n> > structure. This was not the case on first startup, so the maximum\n> > shutter speed reported back via the ControlInfo was incorrect.\n> >\n> > Fix this by setting up the camera mode in the cam-helper before querying\n> > for the max shutter value.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > 1 file changed, 6 insertions(+)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp\n> b/src/ipa/rpi/common/ipa_base.cpp\n> > index 149a133ab662..1d12262bda01 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo\n> &sensorInfo)\n> > mode_.minAnalogueGain =\n> helper_->gain(gainCtrl.min().get());\n> > mode_.maxAnalogueGain =\n> helper_->gain(gainCtrl.max().get());\n> >\n> > + /*\n> > + * We need to give the helper the min/max frame durations so it\n> can calculate\n> > + * the correct exposure limits below.\n> > + */\n> > + helper_->setCameraMode(mode_);\n> > +\n>\n> Don't you end up doing this twice, once here, and once in\n> IpaBase::configure(), which calls IpaBase::setMode() ?\n>\n\nYes, it does happen twice. The other option would be to pass a const\nreference to mode_ into the helper through setMode(), allowing us only do\nit once. But that was a bit more involved over this more trivial fix. I\ncan change to do that if folks prefer.\n\nNaush\n\n\n> > /* Shutter speed is calculated based on the limits of the frame\n> durations. */\n> > mode_.minShutter =\n> helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > mode_.maxShutter = Duration::max();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 51961C3220\n\tfor ;\n\tWed, 1 May 2024 16:04:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7CFB36340B;\n\tWed, 1 May 2024 18:04:42 +0200 (CEST)","from mail-yw1-x1130.google.com (mail-yw1-x1130.google.com\n\t[IPv6:2607:f8b0:4864:20::1130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43947633ED\n\tfor ;\n\tWed, 1 May 2024 18:04:40 +0200 (CEST)","by mail-yw1-x1130.google.com with SMTP id\n\t00721157ae682-61be4b98766so28861527b3.3\n\tfor ;\n\tWed, 01 May 2024 09:04:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Lulq8byO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1714579479; x=1715184279;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=yGwff+NOcG1wiMOfIaHCdqPV0Xuw8t0O5k87/oZtfzA=;\n\tb=Lulq8byOmw891lPBfrFcwX919RO8JKmTukrhL/3kFFxRx6JHWpSzfoxqr6c7058b58\n\t+vbxZZeML8XycWXpPjgtfSBuUYPBCi2mPI+xoxwWKRN8syibHPUzRwDdAA3JX1zQrih9\n\tJkYy3zvUTY4RUV/UmU4vvw2IJFibP1eoQgD1ztxjY6Up8u0SRtTnCeGVdb0mH5HRhAav\n\tpcwFvsKyL52kNjrSW6SxI8ntHd7xPPZIYbXJIQ7I1ge7pgWgHh0iX5LEc5vBgm2XWVDe\n\ts36MQsafaB+QsNgse4vR9yMZhskKb58oFjgbDi7JjmoJzg6jZ2e9M6oEG9IvZM8IifBX\n\tT3pw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1714579479; x=1715184279;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=yGwff+NOcG1wiMOfIaHCdqPV0Xuw8t0O5k87/oZtfzA=;\n\tb=R0E6p1HmcTDUOhOfBvVZgvQs8Fvj0a57PO3fXrP22N2d3uRiAlWnJpfdNPf+2fQLw0\n\tpfhIwT1Z7w4i5CIVjiZEy5xqfpv+Xy8qtbhIsukKwM9R5+0xMOJnxtLS3DbVE6I3u0Ly\n\tGL24lBybKxB9OA1nIH9J4//+lrf2/Dkwaac3mjxRwSwc0ArA34tObZJ8fBViIgPfhAAe\n\tc7xsP0csh62jI+ybgwL52IY1b3hrxM5qrnEZ690VCw20bC/HIYKmxmLYBI53iwDiYS/W\n\trbPXC0C/rV5cpA/rIKqL3Bp4aXjtNfFl2/GsvBJ7D4BPpCsTUSshrWlycFSBVdlawhfm\n\tzrBQ==","X-Gm-Message-State":"AOJu0Yw/99FZkwC5mmGGH+ADTHH5xPtDOk5QBBX0j1TG09aumgxpKYkd\n\tKFos1fz8de6HMzwmcj9K3RFwMoaaQsSfn6r/O/Ji5WWbaJZyueeRvpTmHaU7JlB3L6kjWNyuVdY\n\t2Wxy70QZC5BJGIIQ6ze8wUBw7mcHOqiTyDtV5rA==","X-Google-Smtp-Source":"AGHT+IFVXnr8Vfp5616cWLo/ajv1acvX2IAqEmpiZZYtamLKq/FJbGamZ6D4iv+ePIPyYjBJNeA1SKzF0VVQvFWrWdM=","X-Received":"by 2002:a05:690c:3683:b0:618:2972:ee3b with SMTP id\n\tfu3-20020a05690c368300b006182972ee3bmr3119311ywb.39.1714579479262;\n\tWed, 01 May 2024 09:04:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>","In-Reply-To":"<20240501155309.GD9891@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 1 May 2024 17:04:03 +0100","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Content-Type":"multipart/alternative; boundary=\"000000000000cd1642061766a4b3\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29422,"web_url":"https://patchwork.libcamera.org/comment/29422/","msgid":"<20240504150433.GE24548@pendragon.ideasonboard.com>","date":"2024-05-04T15:04:33","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > The maximum shutter speed calculation in the cam-helper relied on\n> > > the frame duration limits being correctly set in the cam-helper's mode\n> > > structure. This was not the case on first startup, so the maximum\n> > > shutter speed reported back via the ControlInfo was incorrect.\n> > >\n> > > Fix this by setting up the camera mode in the cam-helper before querying\n> > > for the max shutter value.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > 1 file changed, 6 insertions(+)\n> > >\n> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > index 149a133ab662..1d12262bda01 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> > >\n> > > + /*\n> > > + * We need to give the helper the min/max frame durations so it can calculate\n> > > + * the correct exposure limits below.\n> > > + */\n> > > + helper_->setCameraMode(mode_);\n> > > +\n> >\n> > Don't you end up doing this twice, once here, and once in\n> > IpaBase::configure(), which calls IpaBase::setMode() ?\n> \n> Yes, it does happen twice. The other option would be to pass a const\n> reference to mode_ into the helper through setMode(), allowing us only do\n> it once.\n\nI don't think I follow you.\n\n> But that was a bit more involved over this more trivial fix. I\n> can change to do that if folks prefer.\n> \n> > > /* Shutter speed is calculated based on the limits of the frame durations. */\n> > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > mode_.maxShutter = Duration::max();","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 1CDA3BDE6B\n\tfor ;\n\tSat, 4 May 2024 15:04:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16BC662E18;\n\tSat, 4 May 2024 17:04:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8356604C3\n\tfor ;\n\tSat, 4 May 2024 17:04:43 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86C7C552;\n\tSat, 4 May 2024 17:03:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MMmB5FYa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714835024;\n\tbh=8ztUA+sjErW/VyMAxOwiI2FHXF0913LdExW8eP0MsV8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MMmB5FYaKJcHwSKPgpavIkvXDOZPcTSY9M5vYKufFy8Oi1U+L2+ND/5HYyQT/sacj\n\tT5Q6ja+H34Y+BuUW5bLSq0cUSTNU0/uYLnrtfKm7IOpSgCv2NgohURy3JuVxT5tAG6\n\t7Qr7fJ+7/1GevvixX+U5pQ5OZM2l2WCpZvucMeu4=","Date":"Sat, 4 May 2024 18:04:33 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","Message-ID":"<20240504150433.GE24548@pendragon.ideasonboard.com>","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29433,"web_url":"https://patchwork.libcamera.org/comment/29433/","msgid":"","date":"2024-05-07T10:32:54","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Sat, 4 May 2024 at 16:04, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > The maximum shutter speed calculation in the cam-helper relied on\n> > > > the frame duration limits being correctly set in the cam-helper's mode\n> > > > structure. This was not the case on first startup, so the maximum\n> > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > >\n> > > > Fix this by setting up the camera mode in the cam-helper before querying\n> > > > for the max shutter value.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > 1 file changed, 6 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > index 149a133ab662..1d12262bda01 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> > > >\n> > > > + /*\n> > > > + * We need to give the helper the min/max frame durations so it can calculate\n> > > > + * the correct exposure limits below.\n> > > > + */\n> > > > + helper_->setCameraMode(mode_);\n> > > > +\n> > >\n> > > Don't you end up doing this twice, once here, and once in\n> > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> >\n> > Yes, it does happen twice. The other option would be to pass a const\n> > reference to mode_ into the helper through setMode(), allowing us only do\n> > it once.\n>\n> I don't think I follow you.\n\nWhat I mean is that we don't give the cam helper its own copy of the\nmode structure, but pass it a const reference to the IPA's mode\nstructure. This would solve the problem with the calculation here.\n\nRegards,\nNaush\n\n>\n> > But that was a bit more involved over this more trivial fix. I\n> > can change to do that if folks prefer.\n> >\n> > > > /* Shutter speed is calculated based on the limits of the frame durations. */\n> > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > mode_.maxShutter = Duration::max();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 205C2C3220\n\tfor ;\n\tTue, 7 May 2024 10:32:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D108363427;\n\tTue, 7 May 2024 12:32:46 +0200 (CEST)","from mail-yw1-x1134.google.com (mail-yw1-x1134.google.com\n\t[IPv6:2607:f8b0:4864:20::1134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CAFF62C9F\n\tfor ;\n\tTue, 7 May 2024 12:32:44 +0200 (CEST)","by mail-yw1-x1134.google.com with SMTP id\n\t00721157ae682-6153d85053aso20491307b3.0\n\tfor ;\n\tTue, 07 May 2024 03:32:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"iWXUMobH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1715077963; x=1715682763;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=RwirouyCgtPcs+c0DVnI2MMSg6zAk2z9EJnSBKpNzSg=;\n\tb=iWXUMobH+Vr+WlGXna1kU+I2z9DW0ab7EtuqMCrH361hOf8pBBqwGG5Nh/cZSr7S27\n\tHpScAmEu0wXu4Utj6FUkior+JJO316SVabt9Eb2a1INaBLGXXDymEFSxa0IiGZ7IOP3W\n\tCTGBDt8Qu5Yoyx/kSfhiKEo4YRYVqRSVm6BrOlQjpCbvNrykvmn+sUTkT+5wObp0qLdh\n\tB64Wacda4eUdtVbpMCBDYePxT5sD1A7z1o7plfkOnAq3k5j3F3jjAKKOtvo33hLJ68KS\n\ttg0P2iH3HDEuwTI5Lbx9HhvyAfku/mQAtBdLVk+nP+D34grnBc/L6ZAXELclf+8cjTZz\n\tdYZQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1715077963; x=1715682763;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=RwirouyCgtPcs+c0DVnI2MMSg6zAk2z9EJnSBKpNzSg=;\n\tb=OEMuVrst4fDPSjKoI7tFA+6OhgT7YQUQ7iYBnmW58FsMGnRwApH88x75wPAOoCdy3f\n\tpsCmZMMt5nHXRt4RmXLt63OL0Wf8L4nw1FRrjju8bCc2owjLB6jFB2OsjJik4jYm05pX\n\taceZfuc6RHWF5gjW7x/O9yEX3K3KBpR9+is59iSlbhBX4XRhFCtSIb8//zbr8eAf5vJE\n\twP50yl+320CCoVVsdmAu2nYJ9wRavEAeqoAHYvA1dFjEi8AJuUseigbh2g40ip92GZgL\n\tecWmNLXfKW/wRlb/tptVxd0uBoTvfOf2qdH+Ep2jJUzS0/jzN84xIAhdTpM0V2CKoX8E\n\tk2vQ==","X-Gm-Message-State":"AOJu0Yw2uRxDfcGGSDx2qjnWJN6WxqIDL39nbqC7hBTZN4/7r1fp/rbw\n\tl/HXU3mFpno09xVRH24dFZlrAAQytrEufyGFFxOW3uP/wy7caMhPj6ivkeSBob0cdbeeGV3e0dk\n\t8EO/7oTYyufFBUTazMaYWR47awgQhdlvwH4l/DA==","X-Google-Smtp-Source":"AGHT+IHlPHzEyblSN+gGZC6cw3eI1dVdccdno00fQvBs2Pq9NkliGdUZRjcG753aXKsNAQOeuwp+A6vF9Bci6rqp7cM=","X-Received":"by 2002:a81:6041:0:b0:618:79f5:8d32 with SMTP id\n\tu62-20020a816041000000b0061879f58d32mr11888216ywb.48.1715077962534;\n\tTue, 07 May 2024 03:32:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>","In-Reply-To":"<20240504150433.GE24548@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 7 May 2024 11:32:54 +0100","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29474,"web_url":"https://patchwork.libcamera.org/comment/29474/","msgid":"<20240509102314.GC955@pendragon.ideasonboard.com>","date":"2024-05-09T10:23:14","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:\n> On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:\n> > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > The maximum shutter speed calculation in the cam-helper relied on\n> > > > > the frame duration limits being correctly set in the cam-helper's mode\n> > > > > structure. This was not the case on first startup, so the maximum\n> > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > >\n> > > > > Fix this by setting up the camera mode in the cam-helper before querying\n> > > > > for the max shutter value.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > 1 file changed, 6 insertions(+)\n> > > > >\n> > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> > > > >\n> > > > > + /*\n> > > > > + * We need to give the helper the min/max frame durations so it can calculate\n> > > > > + * the correct exposure limits below.\n> > > > > + */\n> > > > > + helper_->setCameraMode(mode_);\n> > > > > +\n> > > >\n> > > > Don't you end up doing this twice, once here, and once in\n> > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > >\n> > > Yes, it does happen twice. The other option would be to pass a const\n> > > reference to mode_ into the helper through setMode(), allowing us only do\n> > > it once.\n> >\n> > I don't think I follow you.\n> \n> What I mean is that we don't give the cam helper its own copy of the\n> mode structure, but pass it a const reference to the IPA's mode\n> structure. This would solve the problem with the calculation here.\n\nIt's probably me, but I still don't get it :-) We have\n\nvoid CamHelper::setCameraMode(const CameraMode &mode)\n{\n\tmode_ = mode;\n\tif (parser_) {\n\t\tparser_->reset();\n\t\tparser_->setBitsPerPixel(mode.bitdepth);\n\t\tparser_->setLineLengthBytes(0); /* We use SetBufferSize. */\n\t}\n}\n\ncalled in IpaBase::configure(). This patch adds another call in\nIpaBase::setMode(). You wrote\n\n> The other option would be to pass a const reference to mode_ into the\n> helper through setMode()\n\nDoesn't this patch does exactly that ? How is it another option ?\n\n/me is confused\n\n> > > But that was a bit more involved over this more trivial fix. I\n> > > can change to do that if folks prefer.\n> > >\n> > > > > /* Shutter speed is calculated based on the limits of the frame durations. */\n> > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > mode_.maxShutter = Duration::max();","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 57746C3226\n\tfor ;\n\tThu, 9 May 2024 10:23:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4BFF163462;\n\tThu, 9 May 2024 12:23:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1D5C6342D\n\tfor ;\n\tThu, 9 May 2024 12:23:23 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1C4DBB3;\n\tThu, 9 May 2024 12:23:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qB0KC4XB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715250200;\n\tbh=9bSzTCefRNuorj0vlOAABnsrq91dXuKO7xyUQhfzcBk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qB0KC4XBnae/OeVJOHGZwrDgt9+MFZrys6QvSNR+J9r/8oP1v2WVOrr0znptwjlbW\n\tzjfuSe/+itk1gEjb16Wo35ugAWFClkngZc4xeF8gT+EcykygUgBLxBPlHAYNCqnpgO\n\tnNHLIEgO1aJlyJl/TZ2kKmNrK7ScWNGKhx9h6bh0=","Date":"Thu, 9 May 2024 13:23:14 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","Message-ID":"<20240509102314.GC955@pendragon.ideasonboard.com>","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29476,"web_url":"https://patchwork.libcamera.org/comment/29476/","msgid":"<171525117586.1857112.11753377429105737446@ping.linuxembedded.co.uk>","date":"2024-05-09T10:39:35","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2024-05-07 11:32:54)\n> Hi Laurent,\n> \n> On Sat, 4 May 2024 at 16:04, Laurent Pinchart\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > The maximum shutter speed calculation in the cam-helper relied on\n> > > > > the frame duration limits being correctly set in the cam-helper's mode\n> > > > > structure. This was not the case on first startup, so the maximum\n> > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > >\n> > > > > Fix this by setting up the camera mode in the cam-helper before querying\n> > > > > for the max shutter value.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > 1 file changed, 6 insertions(+)\n> > > > >\n> > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> > > > >\n> > > > > + /*\n> > > > > + * We need to give the helper the min/max frame durations so it can calculate\n> > > > > + * the correct exposure limits below.\n> > > > > + */\n> > > > > + helper_->setCameraMode(mode_);\n> > > > > +\n> > > >\n> > > > Don't you end up doing this twice, once here, and once in\n> > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > >\n> > > Yes, it does happen twice. The other option would be to pass a const\n> > > reference to mode_ into the helper through setMode(), allowing us only do\n> > > it once.\n> >\n> > I don't think I follow you.\n> \n> What I mean is that we don't give the cam helper its own copy of the\n> mode structure, but pass it a const reference to the IPA's mode\n> structure. This would solve the problem with the calculation here.\n> \n> Regards,\n> Naush\n> \n> >\n> > > But that was a bit more involved over this more trivial fix. I\n> > > can change to do that if folks prefer.\n\nThis is your IPA. I'm happy to merge this if you believe it's ready as\nyou want it.\n\nAcked-by: Kieran Bingham \n\n--\nKieran\n\n\n> > >\n> > > > > /* Shutter speed is calculated based on the limits of the frame durations. */\n> > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > mode_.maxShutter = Duration::max();\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7E37DBDE6B\n\tfor ;\n\tThu, 9 May 2024 10:39:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67C9A6345F;\n\tThu, 9 May 2024 12:39:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2EEAB633FA\n\tfor ;\n\tThu, 9 May 2024 12:39:39 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9854F3D5;\n\tThu, 9 May 2024 12:39:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Hq3Vki9Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715251175;\n\tbh=r204dYUX/xxfs8fAZM+TECl44OltYuRBAMB2r+Vz9ks=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Hq3Vki9YaWiNL+i+QDUjF5a1yry4Jd8Y97KGTR1BYJOZtfs+gxByDPhfMF/lgpN/q\n\tyGiyl7ES8LlU7i5d8KJA0K3WtBBLuSdz8HDy2Zl3y5lskA5XHJd9dCdqDGzMxtbI2R\n\tV9xtQ8omSkkXs5IUjrZOe3QS4VKBbXKriyn6pgXc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","To":"Laurent Pinchart ,\n\tNaushir Patuck ","Date":"Thu, 09 May 2024 11:39:35 +0100","Message-ID":"<171525117586.1857112.11753377429105737446@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29477,"web_url":"https://patchwork.libcamera.org/comment/29477/","msgid":"<20240509104451.GA14520@pendragon.ideasonboard.com>","date":"2024-05-09T10:44:51","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, May 09, 2024 at 11:39:35AM +0100, Kieran Bingham wrote:\n> Quoting Naushir Patuck (2024-05-07 11:32:54)\n> > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:\n> > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > > The maximum shutter speed calculation in the cam-helper relied on\n> > > > > > the frame duration limits being correctly set in the cam-helper's mode\n> > > > > > structure. This was not the case on first startup, so the maximum\n> > > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > > >\n> > > > > > Fix this by setting up the camera mode in the cam-helper before querying\n> > > > > > for the max shutter value.\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > ---\n> > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > > 1 file changed, 6 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> > > > > >\n> > > > > > + /*\n> > > > > > + * We need to give the helper the min/max frame durations so it can calculate\n> > > > > > + * the correct exposure limits below.\n> > > > > > + */\n> > > > > > + helper_->setCameraMode(mode_);\n> > > > > > +\n> > > > >\n> > > > > Don't you end up doing this twice, once here, and once in\n> > > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > > >\n> > > > Yes, it does happen twice. The other option would be to pass a const\n> > > > reference to mode_ into the helper through setMode(), allowing us only do\n> > > > it once.\n> > >\n> > > I don't think I follow you.\n> > \n> > What I mean is that we don't give the cam helper its own copy of the\n> > mode structure, but pass it a const reference to the IPA's mode\n> > structure. This would solve the problem with the calculation here.\n> > \n> > > > But that was a bit more involved over this more trivial fix. I\n> > > > can change to do that if folks prefer.\n> \n> This is your IPA. I'm happy to merge this if you believe it's ready as\n> you want it.\n\nI forgot to clearly mention in my previous reply that I'm OK with that\ntoo. It would be nice to avoid the duplicated call and that would be my\npreference, but I won't block this patch just for this reason.\n\n> Acked-by: Kieran Bingham \n> \n> > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */\n> > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > > mode_.maxShutter = Duration::max();","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 0972CC3226\n\tfor ;\n\tThu, 9 May 2024 10:45:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B2ED63462;\n\tThu, 9 May 2024 12:45:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5A2F633FA\n\tfor ;\n\tThu, 9 May 2024 12:45:00 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 134D83D5;\n\tThu, 9 May 2024 12:44:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EBo9ii/5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715251497;\n\tbh=dBZ64W5tCmEnAP2F2FRc3eyKAkmoqTE+eZodxXy+5Xw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EBo9ii/5D3ues19U2rpdLM4l9rX7dAd/9v8kHPdV/n8EyUTLapnKLkOkb2GykxQa9\n\t5rG9AEAbJgiq3mmxIXa+s694W7lk36wlsp2OOI1RLsl2Q+oLwJgXu64yys4reihp1P\n\t6N1Us6/IZMulsxOUGat5OJ1s8N2bYCzjJLPUJt0o=","Date":"Thu, 9 May 2024 13:44:51 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","Message-ID":"<20240509104451.GA14520@pendragon.ideasonboard.com>","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t\n\t<171525117586.1857112.11753377429105737446@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171525117586.1857112.11753377429105737446@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29478,"web_url":"https://patchwork.libcamera.org/comment/29478/","msgid":"","date":"2024-05-09T10:51:05","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 9 May 2024 at 11:23, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:\n> > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:\n> > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > > The maximum shutter speed calculation in the cam-helper relied on\n> > > > > > the frame duration limits being correctly set in the\n> cam-helper's mode\n> > > > > > structure. This was not the case on first startup, so the maximum\n> > > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > > >\n> > > > > > Fix this by setting up the camera mode in the cam-helper before\n> querying\n> > > > > > for the max shutter value.\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > ---\n> > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > > 1 file changed, 6 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp\n> b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const\n> IPACameraSensorInfo &sensorInfo)\n> > > > > > mode_.minAnalogueGain =\n> helper_->gain(gainCtrl.min().get());\n> > > > > > mode_.maxAnalogueGain =\n> helper_->gain(gainCtrl.max().get());\n> > > > > >\n> > > > > > + /*\n> > > > > > + * We need to give the helper the min/max frame durations\n> so it can calculate\n> > > > > > + * the correct exposure limits below.\n> > > > > > + */\n> > > > > > + helper_->setCameraMode(mode_);\n> > > > > > +\n> > > > >\n> > > > > Don't you end up doing this twice, once here, and once in\n> > > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > > >\n> > > > Yes, it does happen twice. The other option would be to pass a const\n> > > > reference to mode_ into the helper through setMode(), allowing us\n> only do\n> > > > it once.\n> > >\n> > > I don't think I follow you.\n> >\n> > What I mean is that we don't give the cam helper its own copy of the\n> > mode structure, but pass it a const reference to the IPA's mode\n> > structure. This would solve the problem with the calculation here.\n>\n> It's probably me, but I still don't get it :-) We have\n>\n> void CamHelper::setCameraMode(const CameraMode &mode)\n> {\n> mode_ = mode;\n> if (parser_) {\n> parser_->reset();\n> parser_->setBitsPerPixel(mode.bitdepth);\n> parser_->setLineLengthBytes(0); /* We use SetBufferSize. */\n> }\n> }\n>\n> called in IpaBase::configure(). This patch adds another call in\n> IpaBase::setMode(). You wrote\n>\n> > The other option would be to pass a const reference to mode_ into the\n> > helper through setMode()\n>\n> Doesn't this patch does exactly that ? How is it another option ?\n>\n> /me is confused\n\n\nIn CamHelper::setCameraMode(const CameraMode &mode), I would have to store\nthe const reference rather than make a copy of CameraMode mode:\n\ndiff --git a/src/ipa/rpi/cam_helper/cam_helper.h\nb/src/ipa/rpi/cam_helper/cam_helper.h\nindex 58a4b202d5a8..eb2a56484606 100644\n--- a/src/ipa/rpi/cam_helper/cam_helper.h\n+++ b/src/ipa/rpi/cam_helper/cam_helper.h\n@@ -107,7 +107,7 @@ protected:\n Metadata &metadata) const;\n\n std::unique_ptr parser_;\n- CameraMode mode_;\n+ const CameraMode &mode_;\n\nThen in the IPA if I call CamHelper::setCameraMode()\nbefore IpaBase::setMode() + a bit of other refactoring, I think I can\nachieve what I want without a double call.. The \"other bit of refactoring\"\nis the bit I'm a bit hesitant to work on for such a simple fix :)\n\nRegards,\nNaush\n\n\n>\n> > > > But that was a bit more involved over this more trivial fix. I\n> > > > can change to do that if folks prefer.\n> > > >\n> > > > > > /* Shutter speed is calculated based on the limits of the\n> frame durations. */\n> > > > > > mode_.minShutter =\n> helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > > mode_.maxShutter = Duration::max();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 65FC5BDE6B\n\tfor ;\n\tThu, 9 May 2024 10:51:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C4016345F;\n\tThu, 9 May 2024 12:51:38 +0200 (CEST)","from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com\n\t[IPv6:2607:f8b0:4864:20::b34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1211A633FA\n\tfor ;\n\tThu, 9 May 2024 12:51:36 +0200 (CEST)","by mail-yb1-xb34.google.com with SMTP id\n\t3f1490d57ef6-d9b9adaf291so755304276.1\n\tfor ;\n\tThu, 09 May 2024 03:51:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"B5pYXdnf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1715251894; x=1715856694;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=s6cUfqsqIiiYlyb/p4DtHKWlIqloDJ+x9JNn7LgA4gY=;\n\tb=B5pYXdnf3ecGdqvF8DtscvmfC59C2bA9aKGirvGdB2aY5PEfjpvkPBItLe7bJmCWFE\n\tMKX8mccm9C8m9ZTGw34N7CZiLl7ClMlDDHsIqijdki8SGzSYaqoGGP3zsPbfE1DIOMtr\n\tsu8MjyyzJsoQlWlYLibWDnkXKin1n9RoKFRJT2J7k7MQOkeyShIlDZvlU00ngZDNw9qJ\n\t9U3UjoWQPpJhoM2pI2eAo6odGG21Z/MiEbb/7MYQzLfMzy0JgA/gPed2SmMRemoN7bM1\n\tgZmJcLAmIYVZq9Dqi6fTiXyMkzubpD8YmrhSb2XgiSonAkWDQWZxY9vvBoDS9IDqr9hq\n\t8s+A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1715251894; x=1715856694;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=s6cUfqsqIiiYlyb/p4DtHKWlIqloDJ+x9JNn7LgA4gY=;\n\tb=eMIVoaHb6KSBINTvqcjB/YcYONQx4qaqRWhlVNzb3jI2x1znQ+yMjdKP7xTdtrAWJy\n\tHUZ3LdRYlg5EBBJFf77U2gT82Z0h2KQL7LeEk1mqwfqvg3+B2WCfvvIpXSXP2E7sq9z0\n\t1sx1lKmo6xWlFAarMZ9cdNN406EhKTpM037aMBmdzoFRM7k1hcQkC30o2ibUUmmQe4JA\n\trc30Noi2mFSJlYzBxAcXKozSOTuw2GAMu7X3hlKqVbOAk4giMjB0+YFxED80lr55xyeY\n\tFNm4DmiXFvvgM+OfZoTi+tHKDnd6xPQedp2kXAj0JAsCA3VnJVs4k2B/5WCKKG7BPDSz\n\tUfuw==","X-Gm-Message-State":"AOJu0YxeqREBeR9O9Qe9eG8hDx1XpsqrXKXU0JioJhjAh97yTkgZUXFs\n\tEwWSdbJnwMIu6e/bg2viZDjp8KN5Ta+bs3XalvmHuglwaUpjMnDtbM946KBDABqR9Ad8P2Ikuyv\n\tF7DvN8QdNnnqlWvq7jdS8CMqctC35A1VY4aLc9w==","X-Google-Smtp-Source":"AGHT+IEMQ9n2zZ6VFMLJ28+Z429hDBLjd3NC8QSoE0upn4ld1RwdwxntnvnHlsR4YkXUdm7OsawiVBHTrTKnT8dW9Bs=","X-Received":"by 2002:a05:690c:338b:b0:620:34a3:da1 with SMTP id\n\t00721157ae682-62085b162edmr62280757b3.39.1715251894558;\n\tThu, 09 May 2024 03:51:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t\n\t<20240509102314.GC955@pendragon.ideasonboard.com>","In-Reply-To":"<20240509102314.GC955@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 9 May 2024 11:51:05 +0100","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Content-Type":"multipart/alternative; boundary=\"000000000000e039d70618033388\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29479,"web_url":"https://patchwork.libcamera.org/comment/29479/","msgid":"<20240509110111.GD14520@pendragon.ideasonboard.com>","date":"2024-05-09T11:01:11","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, May 09, 2024 at 11:51:05AM +0100, Naushir Patuck wrote:\n> On Thu, 9 May 2024 at 11:23, Laurent Pinchart wrote:\n> > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:\n> > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:\n> > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > > > The maximum shutter speed calculation in the cam-helper relied on\n> > > > > > > the frame duration limits being correctly set in the cam-helper's mode\n> > > > > > > structure. This was not the case on first startup, so the maximum\n> > > > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > > > >\n> > > > > > > Fix this by setting up the camera mode in the cam-helper before querying\n> > > > > > > for the max shutter value.\n> > > > > > >\n> > > > > > > Signed-off-by: Naushir Patuck \n> > > > > > > ---\n> > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > > > 1 file changed, 6 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> > > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> > > > > > >\n> > > > > > > + /*\n> > > > > > > + * We need to give the helper the min/max frame durations so it can calculate\n> > > > > > > + * the correct exposure limits below.\n> > > > > > > + */\n> > > > > > > + helper_->setCameraMode(mode_);\n> > > > > > > +\n> > > > > >\n> > > > > > Don't you end up doing this twice, once here, and once in\n> > > > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > > > >\n> > > > > Yes, it does happen twice. The other option would be to pass a const\n> > > > > reference to mode_ into the helper through setMode(), allowing us only do\n> > > > > it once.\n> > > >\n> > > > I don't think I follow you.\n> > >\n> > > What I mean is that we don't give the cam helper its own copy of the\n> > > mode structure, but pass it a const reference to the IPA's mode\n> > > structure. This would solve the problem with the calculation here.\n> >\n> > It's probably me, but I still don't get it :-) We have\n> >\n> > void CamHelper::setCameraMode(const CameraMode &mode)\n> > {\n> > mode_ = mode;\n> > if (parser_) {\n> > parser_->reset();\n> > parser_->setBitsPerPixel(mode.bitdepth);\n> > parser_->setLineLengthBytes(0); /* We use SetBufferSize. */\n> > }\n> > }\n> >\n> > called in IpaBase::configure(). This patch adds another call in\n> > IpaBase::setMode(). You wrote\n> >\n> > > The other option would be to pass a const reference to mode_ into the\n> > > helper through setMode()\n> >\n> > Doesn't this patch does exactly that ? How is it another option ?\n> >\n> > /me is confused\n> \n> In CamHelper::setCameraMode(const CameraMode &mode), I would have to store\n> the const reference rather than make a copy of CameraMode mode:\n> \n> diff --git a/src/ipa/rpi/cam_helper/cam_helper.h\n> b/src/ipa/rpi/cam_helper/cam_helper.h\n> index 58a4b202d5a8..eb2a56484606 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper.h\n> +++ b/src/ipa/rpi/cam_helper/cam_helper.h\n> @@ -107,7 +107,7 @@ protected:\n> Metadata &metadata) const;\n> \n> std::unique_ptr parser_;\n> - CameraMode mode_;\n> + const CameraMode &mode_;\n> \n> Then in the IPA if I call CamHelper::setCameraMode()\n> before IpaBase::setMode() + a bit of other refactoring, I think I can\n> achieve what I want without a double call.. The \"other bit of refactoring\"\n> is the bit I'm a bit hesitant to work on for such a simple fix :)\n\nAahhh OK, I get it now. The above won't compile though, you can't\nreassign a reference, it can only be initialized at object construction\ntime. You could use a pointer, but I think that will make the code more\ndifficult to understand, with changes being implicitly shared between\ncomponents. Let's stick with this patch for the time being. There's\nprobably a chance to refactor the code to make the interface between the\ncomponents a bit cleaner, but that's a gut feeling at this point. Or\nmaybe a wishful thinking that there is always a way to make pieces fall\nneatly into place :-)\n\n> > > > > But that was a bit more involved over this more trivial fix. I\n> > > > > can change to do that if folks prefer.\n> > > > >\n> > > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */\n> > > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > > > mode_.maxShutter = Duration::max();","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 F3E09C3226\n\tfor ;\n\tThu, 9 May 2024 11:01:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD44F63460;\n\tThu, 9 May 2024 13:01:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A39486342D\n\tfor ;\n\tThu, 9 May 2024 13:01:20 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE184904;\n\tThu, 9 May 2024 13:01:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"d29NzCvO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715252477;\n\tbh=ELEAWjdsXefw8t2qPz0GSmpAPy09IdaMAPlrww7wyds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d29NzCvOgw+qYGUGZSlar7wkVwGv9353w1XubrvzDbJ6SvWTJynBEUG48J6PAGt2o\n\taizD5SUzWGPEMv05WwxUL7fjPvjm4uLa3uG+lS3Cjpgyvmu8tfGkX3Zlhp6Tdv7bNg\n\tdup4MUiXL4Y3Qzw3bgiyec1Qy2MXYN9WOnnu0RUY=","Date":"Thu, 9 May 2024 14:01:11 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","Message-ID":"<20240509110111.GD14520@pendragon.ideasonboard.com>","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t\n\t<20240509102314.GC955@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29480,"web_url":"https://patchwork.libcamera.org/comment/29480/","msgid":"<171525255019.75000.17866885778593935766@ping.linuxembedded.co.uk>","date":"2024-05-09T11:02:30","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-05-09 11:23:14)\n> Hi Naush,\n> \n> On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:\n> > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:\n> > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > > The maximum shutter speed calculation in the cam-helper relied on\n> > > > > > the frame duration limits being correctly set in the cam-helper's mode\n> > > > > > structure. This was not the case on first startup, so the maximum\n> > > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > > >\n> > > > > > Fix this by setting up the camera mode in the cam-helper before querying\n> > > > > > for the max shutter value.\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > ---\n> > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > > 1 file changed, 6 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n> > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n> > > > > >\n> > > > > > + /*\n> > > > > > + * We need to give the helper the min/max frame durations so it can calculate\n> > > > > > + * the correct exposure limits below.\n> > > > > > + */\n> > > > > > + helper_->setCameraMode(mode_);\n> > > > > > +\n> > > > >\n> > > > > Don't you end up doing this twice, once here, and once in\n> > > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > > >\n> > > > Yes, it does happen twice. The other option would be to pass a const\n> > > > reference to mode_ into the helper through setMode(), allowing us only do\n> > > > it once.\n> > >\n> > > I don't think I follow you.\n> > \n> > What I mean is that we don't give the cam helper its own copy of the\n> > mode structure, but pass it a const reference to the IPA's mode\n> > structure. This would solve the problem with the calculation here.\n> \n> It's probably me, but I still don't get it :-) We have\n> \n> void CamHelper::setCameraMode(const CameraMode &mode)\n> {\n> mode_ = mode;\n> if (parser_) {\n> parser_->reset();\n> parser_->setBitsPerPixel(mode.bitdepth);\n> parser_->setLineLengthBytes(0); /* We use SetBufferSize. */\n> }\n> }\n> \n> called in IpaBase::configure(). This patch adds another call in\n> IpaBase::setMode(). You wrote\n> \n> > The other option would be to pass a const reference to mode_ into the\n> > helper through setMode()\n> \n> Doesn't this patch does exactly that ? How is it another option ?\n> \n> /me is confused\n\n\nI can see the duplication.\n\nI guess the question is ... Naush - do you want to remove the second\ncall? I don't see any need to change the constness of it currently?\n\n\nThe call flow I see is:\n\nIpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n\t\t\t ConfigResult *result) \n{\n...\n\t/* Re-assemble camera mode using the sensor info. */\n\tsetMode(sensorInfo);\n\t ->\n\t IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n\t { /* KB: Which with this patch calls */\n\t \t...\n\t\tmode_.minAnalogueGain = helper_->gain(gainCtrl.min().get());\n\t\tmode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get());\n\n+\t\t/*\n+\t\t * We need to give the helper the min/max frame durations so it can calculate\n+\t\t * the correct exposure limits below.\n+\t\t */\n+\t\thelper_->setCameraMode(mode_);\n\n\t\t/* Shutter speed is calculated based on the limits of the frame durations. */\n\t\tmode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n\t\tmode_.maxShutter = Duration::max();\n\t\thelper_->getBlanking(mode_.maxShutter,\n\t\t\t\t mode_.minFrameDuration, mode_.maxFrameDuration);\n\t\t...\n\t }\n\n\t/* KB: And then flows directly into the next two lines to call\n\t * helper_->setCameraMode(mode_); again */\n\n\tmode_.transform = static_cast(params.transform);\n-\n-\t/* Pass the camera mode to the CamHelper to setup algorithms. */\n-\thelper_->setCameraMode(mode_);\n\n...\n}\n\nSo I think the open question is simply, should this patch remove the\nlines I've prefixed with '-' as it has added the lines prefixed with\n'+'....\n\nI think doing so require setting the mode_.transform before calling\nsetMode(sensorInfo), but that looks like it's all it would do?\n\n\nNaush - if you're happy with the patch as is, I can merge it already.\nThere's probably some other interactions we've missed that you have\nbetter visibility on.\n\n--\nKieran\n\n\n\n> \n> > > > But that was a bit more involved over this more trivial fix. I\n> > > > can change to do that if folks prefer.\n> > > >\n> > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */\n> > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > > mode_.maxShutter = Duration::max();\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EE4FFC3226\n\tfor ;\n\tThu, 9 May 2024 11:02:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A23A56345F;\n\tThu, 9 May 2024 13:02:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 620E363433\n\tfor ;\n\tThu, 9 May 2024 13:02:33 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E8555904;\n\tThu, 9 May 2024 13:02:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SRZiuzYa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715252550;\n\tbh=sfuBGOyU3U/naSdNkgOkZuUbl6EcbSwyUt+2efj4OMY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SRZiuzYaq+V5a3uYJ+RvVlQ/G/FuezqS6iPob22J6itNfOXo5Zr6GLB9lvbRXFBsE\n\tZVr4YOfubuJ/NHEF7VYHNbLrtFRwsuhWIE2YBoGonJKrsE9jkr0/APQhCCrolesUE/\n\tKH2Oge5G955/89M60e3ZWVRqvg2vj3WqdRETBSUg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240509102314.GC955@pendragon.ideasonboard.com>","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t\n\t<20240509102314.GC955@pendragon.ideasonboard.com>","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","To":"Laurent Pinchart ,\n\tNaushir Patuck ","Date":"Thu, 09 May 2024 12:02:30 +0100","Message-ID":"<171525255019.75000.17866885778593935766@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29495,"web_url":"https://patchwork.libcamera.org/comment/29495/","msgid":"","date":"2024-05-09T11:43:03","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 9 May 2024 at 12:02, Kieran Bingham \nwrote:\n\n> Quoting Laurent Pinchart (2024-05-09 11:23:14)\n> > Hi Naush,\n> >\n> > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:\n> > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:\n> > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > > > The maximum shutter speed calculation in the cam-helper relied\n> on\n> > > > > > > the frame duration limits being correctly set in the\n> cam-helper's mode\n> > > > > > > structure. This was not the case on first startup, so the\n> maximum\n> > > > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > > > >\n> > > > > > > Fix this by setting up the camera mode in the cam-helper\n> before querying\n> > > > > > > for the max shutter value.\n> > > > > > >\n> > > > > > > Signed-off-by: Naushir Patuck \n> > > > > > > ---\n> > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > > > 1 file changed, 6 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp\n> b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const\n> IPACameraSensorInfo &sensorInfo)\n> > > > > > > mode_.minAnalogueGain =\n> helper_->gain(gainCtrl.min().get());\n> > > > > > > mode_.maxAnalogueGain =\n> helper_->gain(gainCtrl.max().get());\n> > > > > > >\n> > > > > > > + /*\n> > > > > > > + * We need to give the helper the min/max frame\n> durations so it can calculate\n> > > > > > > + * the correct exposure limits below.\n> > > > > > > + */\n> > > > > > > + helper_->setCameraMode(mode_);\n> > > > > > > +\n> > > > > >\n> > > > > > Don't you end up doing this twice, once here, and once in\n> > > > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > > > >\n> > > > > Yes, it does happen twice. The other option would be to pass a\n> const\n> > > > > reference to mode_ into the helper through setMode(), allowing us\n> only do\n> > > > > it once.\n> > > >\n> > > > I don't think I follow you.\n> > >\n> > > What I mean is that we don't give the cam helper its own copy of the\n> > > mode structure, but pass it a const reference to the IPA's mode\n> > > structure. This would solve the problem with the calculation here.\n> >\n> > It's probably me, but I still don't get it :-) We have\n> >\n> > void CamHelper::setCameraMode(const CameraMode &mode)\n> > {\n> > mode_ = mode;\n> > if (parser_) {\n> > parser_->reset();\n> > parser_->setBitsPerPixel(mode.bitdepth);\n> > parser_->setLineLengthBytes(0); /* We use SetBufferSize.\n> */\n> > }\n> > }\n> >\n> > called in IpaBase::configure(). This patch adds another call in\n> > IpaBase::setMode(). You wrote\n> >\n> > > The other option would be to pass a const reference to mode_ into the\n> > > helper through setMode()\n> >\n> > Doesn't this patch does exactly that ? How is it another option ?\n> >\n> > /me is confused\n>\n>\n> I can see the duplication.\n>\n> I guess the question is ... Naush - do you want to remove the second\n> call? I don't see any need to change the constness of it currently?\n>\n>\n> The call flow I see is:\n>\n> IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const\n> ConfigParams ¶ms,\n> ConfigResult *result)\n> {\n> ...\n> /* Re-assemble camera mode using the sensor info. */\n> setMode(sensorInfo);\n> ->\n> IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> { /* KB: Which with this patch calls */\n> ...\n> mode_.minAnalogueGain =\n> helper_->gain(gainCtrl.min().get());\n> mode_.maxAnalogueGain =\n> helper_->gain(gainCtrl.max().get());\n>\n> + /*\n> + * We need to give the helper the min/max frame durations\n> so it can calculate\n> + * the correct exposure limits below.\n> + */\n> + helper_->setCameraMode(mode_);\n>\n> /* Shutter speed is calculated based on the limits of the\n> frame durations. */\n> mode_.minShutter =\n> helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> mode_.maxShutter = Duration::max();\n> helper_->getBlanking(mode_.maxShutter,\n> mode_.minFrameDuration,\n> mode_.maxFrameDuration);\n> ...\n> }\n>\n> /* KB: And then flows directly into the next two lines to call\n> * helper_->setCameraMode(mode_); again */\n>\n> mode_.transform =\n> static_cast(params.transform);\n> -\n> - /* Pass the camera mode to the CamHelper to setup algorithms. */\n> - helper_->setCameraMode(mode_);\n>\n> ...\n> }\n>\n> So I think the open question is simply, should this patch remove the\n> lines I've prefixed with '-' as it has added the lines prefixed with\n> '+'....\n>\n> I think doing so require setting the mode_.transform before calling\n> setMode(sensorInfo), but that looks like it's all it would do?\n>\n\nAlmost but not quite. The copy in the helper will not have the updated\nmin/max shutter.\n\n\n>\n>\n> Naush - if you're happy with the patch as is, I can merge it already.\n> There's probably some other interactions we've missed that you have\n> better visibility on.\n>\n\nI would suggest sticking with the current patch. It can get fiddly quite\nquickly moving these bits of logic around.\n\nRegards,\nNaush\n\n\n>\n> --\n> Kieran\n>\n>\n>\n> >\n> > > > > But that was a bit more involved over this more trivial fix. I\n> > > > > can change to do that if folks prefer.\n> > > > >\n> > > > > > > /* Shutter speed is calculated based on the limits of\n> the frame durations. */\n> > > > > > > mode_.minShutter =\n> helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > > > mode_.maxShutter = Duration::max();\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 22DC3C3226\n\tfor ;\n\tThu, 9 May 2024 11:43:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5850563460;\n\tThu, 9 May 2024 13:43:35 +0200 (CEST)","from mail-yw1-x112e.google.com (mail-yw1-x112e.google.com\n\t[IPv6:2607:f8b0:4864:20::112e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CACD1633FA\n\tfor ;\n\tThu, 9 May 2024 13:43:33 +0200 (CEST)","by mail-yw1-x112e.google.com with SMTP id\n\t00721157ae682-61df903aa05so7153757b3.3\n\tfor ;\n\tThu, 09 May 2024 04:43:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"IBqDNgEF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1715255012; x=1715859812;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=sJPGx7X9PoIRp7ZVmcZ9Sey5lDae1svAKxxPqXsh0sw=;\n\tb=IBqDNgEFBfhPMI2vELOK1Wo2gczzqeVw+W1UGTUq/C0M33df0oN1jNFg+85TUptXdc\n\todb53QrkuPW87kB3peAeNbhumCuek+scjj/JZGuNknB4dRzV40/yu1cHFDm23dp3qhpU\n\tBo7Ax0/ypLDsNNmaUs28s9KSrWsiZhQRl2+vk+rSwDIbuK1ZfXCA/T/qRN08OMsJ8WjU\n\tZaX63mj9FPX/tfRSbiJDKECkYgCueKqMcv5bzMxd5tCDxYWXEMjTw4iiDf74U+UmrYCa\n\tUv9rcC7b/10gboAcJ4D2hj9raU2pRyM0BEaZcNbWGRsSJbJSjZuu3p1hQmLvlG4V8PCT\n\tciYw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1715255012; x=1715859812;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=sJPGx7X9PoIRp7ZVmcZ9Sey5lDae1svAKxxPqXsh0sw=;\n\tb=vmLEGI+KbIdUiHtuSmxrfPvJbp2R/ltrIDTOtcpksmd/tsb5Iv97YRDC3JY4Qzbqlh\n\tZIrndhmHzXZC18uyBNlILkbQ4lSI+JycWM5LjpqD0g4UNjblCaS6wvYgs7XwwlWztx+U\n\tw+Y2T4QJL5BbahMvfCaydCtl9gfVV87Vd32PK3+haBu/m6ROGR1mlbbmkQne7G7joGjO\n\t/vZ/5JDSYPz58DjTy2r1Fgs+4z9PyHyYFUKqpcc+FETu6nDv7jwDwmy/1ApKxdfOZMR0\n\ttm7zLoRTS7dlvQLZxzQLT6x9lQalkGCYbWiE0/ckMQ7JMKMtMSCr0DplURrb+cEPMmn7\n\tGrAQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUsx5hOlTZPi+Y/rBRJLlgdETQFFoTZ9ojrSqZ6UKy1cCxMbNSNKPZbGFOO5gL98qQDlGpx8haqk0VkcY6CXN+rL29szs4TO6JpksYEXL0Sk4+kew==","X-Gm-Message-State":"AOJu0YwbvRBy6NQcEp4XcYgCLjsFWR6P69csKsnUZKLsfkNXFu0Y7G3k\n\tj7Q0dqRIXc7sjgriWs3aN6/L0zMVquSaitu+JAAeqSeOVe7JuHlGgzfSMG2VZS5X9Dyvfbe3SCB\n\tB5QOH+xSg7hRta1tqwc9pWmUJNEme8HvcIAXQQ6DB+1MpV8sDNr0=","X-Google-Smtp-Source":"AGHT+IGLdsgfj9wi8H1zHa/Eb5zF7Nu3fyzV7EqZklCFWYqnGl+0WSqN2TNhcrtIjIQ37DmSRDzH/WkDqDDVvldMpi8=","X-Received":"by 2002:a81:be10:0:b0:61a:ca09:dae3 with SMTP id\n\t00721157ae682-62085da6e92mr55952127b3.26.1715255012692;\n\tThu, 09 May 2024 04:43:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t\n\t<20240509102314.GC955@pendragon.ideasonboard.com>\n\t<171525255019.75000.17866885778593935766@ping.linuxembedded.co.uk>","In-Reply-To":"<171525255019.75000.17866885778593935766@ping.linuxembedded.co.uk>","From":"Naushir Patuck ","Date":"Thu, 9 May 2024 12:43:03 +0100","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","To":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","Content-Type":"multipart/alternative; boundary=\"000000000000bb3293061803ed40\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29496,"web_url":"https://patchwork.libcamera.org/comment/29496/","msgid":"<171525530242.1857112.16136780760001900081@ping.linuxembedded.co.uk>","date":"2024-05-09T11:48:22","subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2024-05-09 12:43:03)\n> On Thu, 9 May 2024 at 12:02, Kieran Bingham \n> wrote:\n> \n> > Quoting Laurent Pinchart (2024-05-09 11:23:14)\n> > > Hi Naush,\n> > >\n> > > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:\n> > > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:\n> > > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:\n> > > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:\n> > > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:\n> > > > > > > > The maximum shutter speed calculation in the cam-helper relied\n> > on\n> > > > > > > > the frame duration limits being correctly set in the\n> > cam-helper's mode\n> > > > > > > > structure. This was not the case on first startup, so the\n> > maximum\n> > > > > > > > shutter speed reported back via the ControlInfo was incorrect.\n> > > > > > > >\n> > > > > > > > Fix this by setting up the camera mode in the cam-helper\n> > before querying\n> > > > > > > > for the max shutter value.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Naushir Patuck \n> > > > > > > > ---\n> > > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++\n> > > > > > > > 1 file changed, 6 insertions(+)\n> > > > > > > >\n> > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp\n> > b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > > index 149a133ab662..1d12262bda01 100644\n> > > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const\n> > IPACameraSensorInfo &sensorInfo)\n> > > > > > > > mode_.minAnalogueGain =\n> > helper_->gain(gainCtrl.min().get());\n> > > > > > > > mode_.maxAnalogueGain =\n> > helper_->gain(gainCtrl.max().get());\n> > > > > > > >\n> > > > > > > > + /*\n> > > > > > > > + * We need to give the helper the min/max frame\n> > durations so it can calculate\n> > > > > > > > + * the correct exposure limits below.\n> > > > > > > > + */\n> > > > > > > > + helper_->setCameraMode(mode_);\n> > > > > > > > +\n> > > > > > >\n> > > > > > > Don't you end up doing this twice, once here, and once in\n> > > > > > > IpaBase::configure(), which calls IpaBase::setMode() ?\n> > > > > >\n> > > > > > Yes, it does happen twice. The other option would be to pass a\n> > const\n> > > > > > reference to mode_ into the helper through setMode(), allowing us\n> > only do\n> > > > > > it once.\n> > > > >\n> > > > > I don't think I follow you.\n> > > >\n> > > > What I mean is that we don't give the cam helper its own copy of the\n> > > > mode structure, but pass it a const reference to the IPA's mode\n> > > > structure. This would solve the problem with the calculation here.\n> > >\n> > > It's probably me, but I still don't get it :-) We have\n> > >\n> > > void CamHelper::setCameraMode(const CameraMode &mode)\n> > > {\n> > > mode_ = mode;\n> > > if (parser_) {\n> > > parser_->reset();\n> > > parser_->setBitsPerPixel(mode.bitdepth);\n> > > parser_->setLineLengthBytes(0); /* We use SetBufferSize.\n> > */\n> > > }\n> > > }\n> > >\n> > > called in IpaBase::configure(). This patch adds another call in\n> > > IpaBase::setMode(). You wrote\n> > >\n> > > > The other option would be to pass a const reference to mode_ into the\n> > > > helper through setMode()\n> > >\n> > > Doesn't this patch does exactly that ? How is it another option ?\n> > >\n> > > /me is confused\n> >\n> >\n> > I can see the duplication.\n> >\n> > I guess the question is ... Naush - do you want to remove the second\n> > call? I don't see any need to change the constness of it currently?\n> >\n> >\n> > The call flow I see is:\n> >\n> > IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const\n> > ConfigParams ¶ms,\n> > ConfigResult *result)\n> > {\n> > ...\n> > /* Re-assemble camera mode using the sensor info. */\n> > setMode(sensorInfo);\n> > ->\n> > IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > { /* KB: Which with this patch calls */\n> > ...\n> > mode_.minAnalogueGain =\n> > helper_->gain(gainCtrl.min().get());\n> > mode_.maxAnalogueGain =\n> > helper_->gain(gainCtrl.max().get());\n> >\n> > + /*\n> > + * We need to give the helper the min/max frame durations\n> > so it can calculate\n> > + * the correct exposure limits below.\n> > + */\n> > + helper_->setCameraMode(mode_);\n> >\n> > /* Shutter speed is calculated based on the limits of the\n> > frame durations. */\n> > mode_.minShutter =\n> > helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > mode_.maxShutter = Duration::max();\n> > helper_->getBlanking(mode_.maxShutter,\n> > mode_.minFrameDuration,\n> > mode_.maxFrameDuration);\n> > ...\n> > }\n> >\n> > /* KB: And then flows directly into the next two lines to call\n> > * helper_->setCameraMode(mode_); again */\n> >\n> > mode_.transform =\n> > static_cast(params.transform);\n> > -\n> > - /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > - helper_->setCameraMode(mode_);\n> >\n> > ...\n> > }\n> >\n> > So I think the open question is simply, should this patch remove the\n> > lines I've prefixed with '-' as it has added the lines prefixed with\n> > '+'....\n> >\n> > I think doing so require setting the mode_.transform before calling\n> > setMode(sensorInfo), but that looks like it's all it would do?\n> >\n> \n> Almost but not quite. The copy in the helper will not have the updated\n> min/max shutter.\n> \n> \n> >\n> >\n> > Naush - if you're happy with the patch as is, I can merge it already.\n> > There's probably some other interactions we've missed that you have\n> > better visibility on.\n> >\n> \n> I would suggest sticking with the current patch. It can get fiddly quite\n> quickly moving these bits of logic around.\n\nSold, merging.\n--\nKieran\n\n> \n> Regards,\n> Naush\n> \n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> >\n> > >\n> > > > > > But that was a bit more involved over this more trivial fix. I\n> > > > > > can change to do that if folks prefer.\n> > > > > >\n> > > > > > > > /* Shutter speed is calculated based on the limits of\n> > the frame durations. */\n> > > > > > > > mode_.minShutter =\n> > helper_->exposure(shutterCtrl.min().get(), mode_.minLineLength);\n> > > > > > > > mode_.maxShutter = Duration::max();\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\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 9132FBDE6B\n\tfor ;\n\tThu, 9 May 2024 11:48:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9D2E63460;\n\tThu, 9 May 2024 13:48:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A520A6342D\n\tfor ;\n\tThu, 9 May 2024 13:48:25 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EFA41904;\n\tThu, 9 May 2024 13:48:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Yk19qM9T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715255302;\n\tbh=q1YtD5uH8pUWBVXBRpFx+kUMPaOBsDNlK8VYDWXPYQI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Yk19qM9TPAZRlgOESHkJbz+Lw5YrOqf1BpwXyBMraNJeLWTg3D2vuap57HDTxhg7D\n\tEidunVCQD/guXEJ/6e4LtgLJM1rPiGGp+y7UlESBlwDJpGXH2yvz+SWCgMqFOiNAJc\n\txp7P0TVO8rAsn07Z19BL6KV0KluepuD5B8nKOxq0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240426111815.10763-1-naush@raspberrypi.com>\n\t<20240501155309.GD9891@pendragon.ideasonboard.com>\n\t\n\t<20240504150433.GE24548@pendragon.ideasonboard.com>\n\t\n\t<20240509102314.GC955@pendragon.ideasonboard.com>\n\t<171525255019.75000.17866885778593935766@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed","From":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com","To":"Naushir Patuck ","Date":"Thu, 09 May 2024 12:48:22 +0100","Message-ID":"<171525530242.1857112.16136780760001900081@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]