[{"id":21455,"web_url":"https://patchwork.libcamera.org/comment/21455/","msgid":"<163827629113.3059017.14040566098093995578@Monstersaurus>","date":"2021-11-30T12:44:51","subject":"Re: [libcamera-devel] [PATCH v6 4/4] ipu3: ipa: Allow IPA to apply\n\tcontrols to the lens device","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Han-Lin Chen (2021-11-30 10:51:57)\n> Allow IPA to apply controls to the lens device.\n> \n> Signed-off-by: Han-Lin Chen \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 1 +\n> src/libcamera/pipeline/ipu3/cio2.h | 3 +++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++--\n> 3 files changed, 14 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 59dda56b..ff795310 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/camera_lens.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/media_device.h\"\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index ba8f0052..635566c8 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -18,6 +18,7 @@\n> \n> namespace libcamera {\n> \n> +class CameraLens;\n> class CameraSensor;\n> class FrameBuffer;\n> class MediaDevice;\n> @@ -52,6 +53,7 @@ public:\n> int stop();\n> \n> CameraSensor *sensor() { return sensor_.get(); }\n> + CameraLens *lens() { return lens_.get(); }\n> const CameraSensor *sensor() const { return sensor_.get(); }\n> \n> FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);\n> @@ -67,6 +69,7 @@ private:\n> void cio2BufferReady(FrameBuffer *buffer);\n> \n> std::unique_ptr sensor_;\n> + std::unique_ptr lens_;\n\nI had assumed we'd model a lens as part of a sensor (i.e. create it in\nthe CameraSensor class).\n\nIs there ever a case where we would need to keep the lens controller\nseparate from the Sensor?\n\n\n\n> std::unique_ptr csi2_;\n> std::unique_ptr output_;\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c65afdb2..6e04ec8f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> \n> #include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/camera_lens.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/delayed_controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> @@ -1238,8 +1239,15 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> {\n> switch (action.op) {\n> case ipa::ipu3::ActionSetSensorControls: {\n> - const ControlList &controls = action.sensorControls;\n> - delayedCtrls_->push(controls);\n> + const ControlList &sensorControls = action.sensorControls;\n> + delayedCtrls_->push(sensorControls);\n> +\n> + const ControlList lensControls = action.lensControls;\n> + const ControlValue &focusValue =\n> + lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> + if (!focusValue.isNone() && cio2_.lens())\n> + cio2_.lens()->setFocusPostion(focusValue.get());\n> +\n\nAnd if this was handled by the CameraSensor class, it gets handled for\nall pipelines (that use the CameraSensor class...)\n\nThis will have to be repeated ... somewhat verbatim in the RkISP\npipeline handler right?\n\nAnyway, this progresses AF at least, so it's something we can build\nupon but I really think this should get reworked sometime.\n\nA tentative:\n\n\nReviewed-by: Kieran Bingham \n\nOf course, this doesn't actually have a lens connected yet, but given we\nhave three developers trying to work on the same thing, I think we need\nto get a base ground to continue so I wouldn't object to merging this to\nsupport ongoing development.\n\n\n> break;\n> }\n> case ipa::ipu3::ActionParamFilled: {\n> -- \n> 2.34.0.rc2.393.gf8c9666880-goog\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 62C15BDB13\n\tfor ;\n\tTue, 30 Nov 2021 12:44:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1907D6071C;\n\tTue, 30 Nov 2021 13:44:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46C58605C4\n\tfor ;\n\tTue, 30 Nov 2021 13:44:53 +0100 (CET)","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 EFA568F0;\n\tTue, 30 Nov 2021 13:44:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i4EiHEPn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638276293;\n\tbh=fa3T1BhU1UT6Zd3csLgEWsncmzaPzSYNOy0GkwZOHFE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=i4EiHEPn98vk8BfDl0/hSNavgU21oTYjKvYtzSbFglo6SoHvqpPixWPyhuwd89KXP\n\txTjXvo4YcAOiuTpxR6mf3p/EQBKKLEgWpNBexNmNnwbza9h5Z2kYd7rse1er0mN+nH\n\tiiPFIm/gPv8YHH6OR09ntX7dEkc9+ZNJLgnT1aCk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211130105157.203856-5-hanlinchen@chromium.org>","References":"<20211130105157.203856-1-hanlinchen@chromium.org>\n\t<20211130105157.203856-5-hanlinchen@chromium.org>","From":"Kieran Bingham ","To":"Han-Lin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 30 Nov 2021 12:44:51 +0000","Message-ID":"<163827629113.3059017.14040566098093995578@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 4/4] ipu3: ipa: Allow IPA to apply\n\tcontrols to the lens device","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":21548,"web_url":"https://patchwork.libcamera.org/comment/21548/","msgid":"","date":"2021-12-02T13:58:49","subject":"Re: [libcamera-devel] [PATCH v6 4/4] ipu3: ipa: Allow IPA to apply\n\tcontrols to the lens device","submitter":{"id":98,"url":"https://patchwork.libcamera.org/api/people/98/","name":"Hanlin Chen","email":"hanlinchen@chromium.org"},"content":"Hi Kieran,\nThanks for comments.\n\nOn Tue, Nov 30, 2021 at 8:44 PM Kieran Bingham\n wrote:\n>\n> Quoting Han-Lin Chen (2021-11-30 10:51:57)\n> > Allow IPA to apply controls to the lens device.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 1 +\n> > src/libcamera/pipeline/ipu3/cio2.h | 3 +++\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++--\n> > 3 files changed, 14 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 59dda56b..ff795310 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/camera_lens.h\"\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/framebuffer.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > index ba8f0052..635566c8 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > @@ -18,6 +18,7 @@\n> >\n> > namespace libcamera {\n> >\n> > +class CameraLens;\n> > class CameraSensor;\n> > class FrameBuffer;\n> > class MediaDevice;\n> > @@ -52,6 +53,7 @@ public:\n> > int stop();\n> >\n> > CameraSensor *sensor() { return sensor_.get(); }\n> > + CameraLens *lens() { return lens_.get(); }\n> > const CameraSensor *sensor() const { return sensor_.get(); }\n> >\n> > FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);\n> > @@ -67,6 +69,7 @@ private:\n> > void cio2BufferReady(FrameBuffer *buffer);\n> >\n> > std::unique_ptr sensor_;\n> > + std::unique_ptr lens_;\n>\n> I had assumed we'd model a lens as part of a sensor (i.e. create it in\n> the CameraSensor class).\n>\n> Is there ever a case where we would need to keep the lens controller\n> separate from the Sensor?\n>\n>\n>\n> > std::unique_ptr csi2_;\n> > std::unique_ptr output_;\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c65afdb2..6e04ec8f 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -24,6 +24,7 @@\n> > #include \n> >\n> > #include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/camera_lens.h\"\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/delayed_controls.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > @@ -1238,8 +1239,15 @@ void IPU3CameraData::queueFrameAction(unsigned int id,\n> > {\n> > switch (action.op) {\n> > case ipa::ipu3::ActionSetSensorControls: {\n> > - const ControlList &controls = action.sensorControls;\n> > - delayedCtrls_->push(controls);\n> > + const ControlList &sensorControls = action.sensorControls;\n> > + delayedCtrls_->push(sensorControls);\n> > +\n> > + const ControlList lensControls = action.lensControls;\n> > + const ControlValue &focusValue =\n> > + lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > + if (!focusValue.isNone() && cio2_.lens())\n> > + cio2_.lens()->setFocusPostion(focusValue.get());\n> > +\n>\n> And if this was handled by the CameraSensor class, it gets handled for\n> all pipelines (that use the CameraSensor class...)\n>\n> This will have to be repeated ... somewhat verbatim in the RkISP\n> pipeline handler right?\n>\n> Anyway, this progresses AF at least, so it's something we can build\n> upon but I really think this should get reworked sometime.\nI agree. Since there are similar discussion in Daniel's series too,\nlet's move the lens to CameraSensor.\n>\n> A tentative:\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n> Of course, this doesn't actually have a lens connected yet, but given we\n> have three developers trying to work on the same thing, I think we need\n> to get a base ground to continue so I wouldn't object to merging this to\n> support ongoing development.\n>\n>\n> > break;\n> > }\n> > case ipa::ipu3::ActionParamFilled: {\n> > --\n> > 2.34.0.rc2.393.gf8c9666880-goog\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 586ACBDB13\n\tfor ;\n\tThu, 2 Dec 2021 13:59:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9DF36082A;\n\tThu, 2 Dec 2021 14:59:02 +0100 (CET)","from mail-oi1-x236.google.com (mail-oi1-x236.google.com\n\t[IPv6:2607:f8b0:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A2466011A\n\tfor ;\n\tThu, 2 Dec 2021 14:59:01 +0100 (CET)","by mail-oi1-x236.google.com with SMTP id o4so55627216oia.10\n\tfor ;\n\tThu, 02 Dec 2021 05:59:01 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"hSuLxzrp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=GiAk0wo0aVyaML2F0qGwHL+8CRQF7/AT4S3omkszzq8=;\n\tb=hSuLxzrp7GvEqVwWJ1Qy6zWF1QymRt9HWlmtnjlqr4BaLKPusBVYa3p/pY1VXUYouM\n\tikx879yni8uM8ZIX3bL8EX0iPrHs/vrpn3U+f5D9ZvbQhAcX0GxdJnKkSyhOrtouzHUl\n\tFbcQDXy5KqwL+uYfydhUHJeyWBq0VFcdXpNak=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=GiAk0wo0aVyaML2F0qGwHL+8CRQF7/AT4S3omkszzq8=;\n\tb=3u0hb6oytGWWODQggVPKTn0Zq7Bupy+EObtdm0hQpMTmDwaDTgt1dycgo46pRwVYqc\n\tnKvXByI3hMQKa/ulZgDR/c7EZCOUIV+eDF4xqnpFO0vmkiNMdXRCkuKFD6/5z7KAtEFV\n\tcfYc7ydi9IJi4wybz5WLyJ4ayAlktSie8pBSFl56ifwiQ83U3SqRu9Yz9OZqdjlut/Uo\n\trlccrUYg4QaTV3V/bZlkjGeLvIaEdf09mNqMR4rKS0UP1q+zxvZo1i6gigp0xw9HRUea\n\tqtTKNmQIOdY5108wjPGb1jEDwlg8EKlhHjAqCfluLQeDOFx0k5QVKn/MudrfdOvktPbR\n\tlAdg==","X-Gm-Message-State":"AOAM533GX4J6NtsbvV8XkqbYiGRm8vcmDI0jkd9USx/8BommExYnv5Xy\n\tQW25pkIWJ5hklRNOmpCiEiQdC2Klqje+ZNERkN4M/37F7dM=","X-Google-Smtp-Source":"ABdhPJz6aWaZBA2GhiZ3OObmkNL0tgT+qAzYUrLrUz91TyHnNoEA4+k5HFLqGAJi1eci9gc1+hYmGh+DdGUJVKT5wRs=","X-Received":"by 2002:a54:4595:: with SMTP id\n\tz21mr4411968oib.169.1638453540133; \n\tThu, 02 Dec 2021 05:59:00 -0800 (PST)","MIME-Version":"1.0","References":"<20211130105157.203856-1-hanlinchen@chromium.org>\n\t<20211130105157.203856-5-hanlinchen@chromium.org>\n\t<163827629113.3059017.14040566098093995578@Monstersaurus>","In-Reply-To":"<163827629113.3059017.14040566098093995578@Monstersaurus>","From":"Hanlin Chen ","Date":"Thu, 2 Dec 2021 21:58:49 +0800","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 4/4] ipu3: ipa: Allow IPA to apply\n\tcontrols to the lens device","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]