[{"id":23695,"web_url":"https://patchwork.libcamera.org/comment/23695/","msgid":"<165662725317.2049236.6515418814536107551@Monstersaurus>","date":"2022-06-30T22:14:13","subject":"Re: [libcamera-devel] [PATCH v3 21/23] ipa: ipu3: Add and use\n\tLensFocusAbsolute control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:39:00)\n> Add to the internal controls LensFocusAbsolute as a draft control\n> as it is currently identical to V4L2_CID_FOCUS_ABSOLUTE.\n> \n> Use the newly introduced control in the IPU3 pipeline handler and IPA\n> module.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/ipu3.mojom | 3 +--\n> src/ipa/ipu3/ipu3.cpp | 7 ++-----\n> src/libcamera/internal_control_ids.yaml | 13 +++++++++++++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++--------\n> 4 files changed, 21 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 64fe65fdd5fc..bfeadc58c52a 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -35,8 +35,7 @@ interface IPAIPU3Interface {\n> };\n> \n> interface IPAIPU3EventInterface {\n> - setSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n> - libcamera.ControlList lensControls);\n> + setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> paramsBufferReady(uint32 frame);\n> metadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 6d622b4c290b..2f158df367a8 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -541,12 +541,9 @@ void IPAIPU3::setControls(unsigned int frame, const IPAActiveState &state)\n> ControlList ctrls(controls::internal::controls);\n> ctrls.set(controls::internal::ExposureTime, state.agc.exposure);\n> ctrls.set(controls::internal::AnalogueGain, static_cast(state.agc.gain));\n> + ctrls.set(controls::internal::draft::LensFocusAbsolute, state.af.focus);\n> \n> - ControlList lensCtrls(controls::controls);\n> - lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> - static_cast(state.af.focus));\n> -\n> - setSensorControls.emit(frame, ctrls, lensCtrls);\n> + setSensorControls.emit(frame, ctrls);\n> }\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml\n> index 6d3775afcf67..804c227213a7 100644\n> --- a/src/libcamera/internal_control_ids.yaml\n> +++ b/src/libcamera/internal_control_ids.yaml\n> @@ -24,4 +24,17 @@ controls:\n> description: |\n> The sensor analogue gain value.\n> \n> + # ----------------------------------------------------------------------------\n> + # Draft controls section\n> +\n> + - LensFocusAbsolute:\n> + type: int32_t\n> + draft: true\n> + description: |\n> + This control sets the focal point of the camera to the specified\n> + position. The unit is undefined. Positive values set the focus closer to\n> + the camera, negative values towards infinity.\n\nI think this is something that we should be returning in\nrequest->metadata().\n\nSo that means it can't be an internal only control ?\n\nWe should define the units too. I thought David had a definition based\naround a calibrated/known infinity point?\n\n> +\n> + Currently identical to V4L2_CID_FOCUS_ABSOLUTE.\n> +\n> ...\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 55b96137f065..1dbcdd0b56e6 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -86,8 +86,7 @@ public:\n> private:\n> void metadataReady(unsigned int id, const ControlList &metadata);\n> void paramsBufferReady(unsigned int id);\n> - void setSensorControls(unsigned int id, const ControlList &sensorControls,\n> - const ControlList &lensControls);\n> + void setSensorControls(unsigned int id, const ControlList &sensorControls);\n> };\n> \n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -1247,8 +1246,7 @@ int IPU3CameraData::loadIPA()\n> }\n> \n> void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> - const ControlList &sensorControls,\n> - const ControlList &lensControls)\n> + const ControlList &sensorControls)\n> {\n> CameraSensor *sensor = cio2_.sensor();\n> \n> @@ -1258,12 +1256,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> if (!focusLens)\n> return;\n> \n> - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> + if (!sensorControls.contains(controls::internal::draft::LensFocusAbsolute))\n> return;\n> \n> - const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> -\n> - focusLens->setFocusPosition(focusValue.get());\n> + int32_t focusAbsolute = sensorControls.get(controls::internal::draft::LensFocusAbsolute);\n> + focusLens->setFocusPosition(focusAbsolute);\n\nAnd I think conversion between our defined units and the units specific to a\nlens driver should then be handled in the CameraLens class, so passing\nin our 'defined' libcamera units type here should be fine.\n\n\n> }\n> \n> void IPU3CameraData::paramsBufferReady(unsigned int id)\n> -- \n> 2.36.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 AECF3BD808\n\tfor ;\n\tThu, 30 Jun 2022 22:15:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D386E6564E;\n\tFri, 1 Jul 2022 00:15:21 +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 64E0A60412\n\tfor ;\n\tFri, 1 Jul 2022 00:15:19 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC81525C;\n\tFri, 1 Jul 2022 00:15:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656627321;\n\tbh=vFH1cRTRyKwHTZQtEuB9WTACbMLFeqZV4B4p3ppbFRo=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=mNsXPPOa5yn4euE3awUGYai3E1vs5+oTjRrfQzzFfVcvbuKG2CS5OZBekM0pS/fhd\n\tyUXl0tgpT9q1a0+ptuF3pAeFfNGl9Fv2z06Dy0Zygpjj/aphSjFm3X/+OUjCm+bPB9\n\tvLwf9hntGtYeXhYzciH96Mjwv8jUzSPZwJVKx4UsRHKpLiMnKyDUzPVB7ZGLsKo5F1\n\tAO1dGldENOWJRZK8+d1bxpdY+ETLzBx0J4ni2fUP7B8fg5XaM776zssQecX0HRF6KE\n\t8xv/nXzohD3tyE8Mwz+rqEfvHnNzi85z/r4AcU/e++ZVjU0XNA9miZklKd7B76enXP\n\tUxAIcfceouJ9g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656627319;\n\tbh=vFH1cRTRyKwHTZQtEuB9WTACbMLFeqZV4B4p3ppbFRo=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=nFeEcO4p5STXRNCCAQNVe9cVMz/pNXxm2P3buMZ8lXcGHY/fsMFjCdfhDMV6mcOp3\n\t+tnH2PXgCZgRd70cjCis9CM737iprwmt8UHQs77QUVrvKT0JLtFehBe3KUoS+G+ieN\n\tO/8yQQrbLDuAe2T8uRRwmtUXOxhFB22Y54cEP0Gs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nFeEcO4p\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220630133902.321099-22-jacopo@jmondi.org>","References":"<20220630133902.321099-1-jacopo@jmondi.org>\n\t<20220630133902.321099-22-jacopo@jmondi.org>","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Thu, 30 Jun 2022 23:14:13 +0100","Message-ID":"<165662725317.2049236.6515418814536107551@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 21/23] ipa: ipu3: Add and use\n\tLensFocusAbsolute control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23705,"web_url":"https://patchwork.libcamera.org/comment/23705/","msgid":"<20220701083851.wrxuk3yhetypsyyd@uno.localdomain>","date":"2022-07-01T08:38:51","subject":"Re: [libcamera-devel] [PATCH v3 21/23] ipa: ipu3: Add and use\n\tLensFocusAbsolute control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Thu, Jun 30, 2022 at 11:14:13PM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:39:00)\n> > Add to the internal controls LensFocusAbsolute as a draft control\n> > as it is currently identical to V4L2_CID_FOCUS_ABSOLUTE.\n> >\n> > Use the newly introduced control in the IPU3 pipeline handler and IPA\n> > module.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 3 +--\n> > src/ipa/ipu3/ipu3.cpp | 7 ++-----\n> > src/libcamera/internal_control_ids.yaml | 13 +++++++++++++\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++--------\n> > 4 files changed, 21 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > index 64fe65fdd5fc..bfeadc58c52a 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -35,8 +35,7 @@ interface IPAIPU3Interface {\n> > };\n> >\n> > interface IPAIPU3EventInterface {\n> > - setSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n> > - libcamera.ControlList lensControls);\n> > + setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > paramsBufferReady(uint32 frame);\n> > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > };\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 6d622b4c290b..2f158df367a8 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -541,12 +541,9 @@ void IPAIPU3::setControls(unsigned int frame, const IPAActiveState &state)\n> > ControlList ctrls(controls::internal::controls);\n> > ctrls.set(controls::internal::ExposureTime, state.agc.exposure);\n> > ctrls.set(controls::internal::AnalogueGain, static_cast(state.agc.gain));\n> > + ctrls.set(controls::internal::draft::LensFocusAbsolute, state.af.focus);\n> >\n> > - ControlList lensCtrls(controls::controls);\n> > - lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > - static_cast(state.af.focus));\n> > -\n> > - setSensorControls.emit(frame, ctrls, lensCtrls);\n> > + setSensorControls.emit(frame, ctrls);\n> > }\n> >\n> > } /* namespace ipa::ipu3 */\n> > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml\n> > index 6d3775afcf67..804c227213a7 100644\n> > --- a/src/libcamera/internal_control_ids.yaml\n> > +++ b/src/libcamera/internal_control_ids.yaml\n> > @@ -24,4 +24,17 @@ controls:\n> > description: |\n> > The sensor analogue gain value.\n> >\n> > + # ----------------------------------------------------------------------------\n> > + # Draft controls section\n> > +\n> > + - LensFocusAbsolute:\n> > + type: int32_t\n> > + draft: true\n> > + description: |\n> > + This control sets the focal point of the camera to the specified\n> > + position. The unit is undefined. Positive values set the focus closer to\n> > + the camera, negative values towards infinity.\n>\n> I think this is something that we should be returning in\n> request->metadata().\n>\n> So that means it can't be an internal only control ?\n>\n> We should define the units too. I thought David had a definition based\n> around a calibrated/known infinity point?\n>\n\nWe're missing a lot of pieces yet. We have defined a unit for\nLensPosition which requires a clear way to measure/calibrate the lens,\nand for internal usage we have no way yet to translate the generic\ncontrol to the sensor-specific value.\n\nSo this is identical to the V4L2 version, as that's what the IPA uses\nat the moment.\n\n> > +\n> > + Currently identical to V4L2_CID_FOCUS_ABSOLUTE.\n> > +\n> > ...\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 55b96137f065..1dbcdd0b56e6 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -86,8 +86,7 @@ public:\n> > private:\n> > void metadataReady(unsigned int id, const ControlList &metadata);\n> > void paramsBufferReady(unsigned int id);\n> > - void setSensorControls(unsigned int id, const ControlList &sensorControls,\n> > - const ControlList &lensControls);\n> > + void setSensorControls(unsigned int id, const ControlList &sensorControls);\n> > };\n> >\n> > class IPU3CameraConfiguration : public CameraConfiguration\n> > @@ -1247,8 +1246,7 @@ int IPU3CameraData::loadIPA()\n> > }\n> >\n> > void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> > - const ControlList &sensorControls,\n> > - const ControlList &lensControls)\n> > + const ControlList &sensorControls)\n> > {\n> > CameraSensor *sensor = cio2_.sensor();\n> >\n> > @@ -1258,12 +1256,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> > if (!focusLens)\n> > return;\n> >\n> > - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> > + if (!sensorControls.contains(controls::internal::draft::LensFocusAbsolute))\n> > return;\n> >\n> > - const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > -\n> > - focusLens->setFocusPosition(focusValue.get());\n> > + int32_t focusAbsolute = sensorControls.get(controls::internal::draft::LensFocusAbsolute);\n> > + focusLens->setFocusPosition(focusAbsolute);\n>\n> And I think conversion between our defined units and the units specific to a\n> lens driver should then be handled in the CameraLens class, so passing\n> in our 'defined' libcamera units type here should be fine.\n>\n>\n> > }\n> >\n> > void IPU3CameraData::paramsBufferReady(unsigned int id)\n> > --\n> > 2.36.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 91006BE173\n\tfor ;\n\tFri, 1 Jul 2022 08:38:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED37F6564B;\n\tFri, 1 Jul 2022 10:38:56 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17E3B6054A\n\tfor ;\n\tFri, 1 Jul 2022 10:38:55 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4C7EF200014;\n\tFri, 1 Jul 2022 08:38:53 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656664737;\n\tbh=Mrgjqi85MH8y/OsvknXvjWDPEkJvLdRaVQY6AERBbMI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HC5GTtGFkh0qunT28vLa6AaCaJGkSh7WOsQiI8e7tBg5oNZiZ7gugBjeNbemAWES+\n\tkkkFgSl0fmxdlG+3KvsC5nFWet1/ik5/+nPyhLh3z4oRMJ1luLcYi66A+DMZ4iEYCU\n\tB5SD89snSdSN6W1DYPE8jVAeog/UGY0xOuWx3t1e4d8RRPe4w/52Jxyi0dLXWaXQEC\n\t+WGM6OOXZCv4eziy3lw/sXd6qCsiG96FbDlPyUB2VwtS12EoKYo2lubx377ACZgZlm\n\tBwP5c9gf+J3LWkRN0lJFI408hOo2vODV4gj0i+Y9ZOCbT/hYb60jk6y0U2J6RXbaxR\n\tdDNu6jrRrg4+Q==","Date":"Fri, 1 Jul 2022 10:38:51 +0200","To":"Kieran Bingham ","Message-ID":"<20220701083851.wrxuk3yhetypsyyd@uno.localdomain>","References":"<20220630133902.321099-1-jacopo@jmondi.org>\n\t<20220630133902.321099-22-jacopo@jmondi.org>\n\t<165662725317.2049236.6515418814536107551@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165662725317.2049236.6515418814536107551@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 21/23] ipa: ipu3: Add and use\n\tLensFocusAbsolute control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]