[{"id":14397,"web_url":"https://patchwork.libcamera.org/comment/14397/","msgid":"","date":"2020-12-29T17:10:08","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: camera_sensor:\n\tRename controls() method","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-12-28 17:55:58 +0100, Jacopo Mondi wrote:\n> The CameraSensor::controls() methods returns the information relative to\n> the V4L2 controls registered by the sensor sub-device driver. Its\n> current use is to inform the IPA module of two pipelines (RkISP1 and\n> VIMC) about the V4L2 controls limits.\n> \n> The CameraSensor class has a controls_ field, which is instead the\n> ControlInfoMap of libcamera controls registered by the CameraSensor\n> class and meant to be exported as Camera controls.\n> \n> To prepare to register libcamera controls in the CameraSensor::controls_\n> info map, and remove any ambiguity on the intended usage of\n> CameraSensor::controls(), rename the method in\n> CameraSensor::subdevControls() and update its users in the code base.\n> \n> Reviewed-by: Paul Elder \n> Signed-off-by: Jacopo Mondi \n\nThe patch in itself is good but I'm not sure I understand the direction \nwe are taking with CameraSensor. I understand we should not expose any \nV4L2 controls to applications but from the pipeline to core helpers (and \nIPA) do we really wish to use libcamera controls? I can't see that we \nwill have core helpers or IPA that would work with both V4L2 and \nsomething new. In my view such interfaces should be kept as close to the \ndrivers it interacts with, in this case V4L2.\n\nI might be missing some use-case where mixing libcamera and V4L2 \ncontrols in helpers and lowlevel interfaces are desired but I can't \nreally see it. At first I thought maybe we would have some controls that \nsimply could be passed from application to CameraSensor but for all \ncases I can think of I can see the possibility for an IPA to be involved \nand then I get confused as we mix and match between the two already too \nmuch in my taste ;-)\n\nMaybe the right solution is to move even further in the direction taken \nhere with the goal to not expose V4L2 controls outside the V4L2 classes. \nIn either case I would like to understand what we aim for.\n\nFor this rename change alone,\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/internal/camera_sensor.h | 2 +-\n> src/libcamera/camera_sensor.cpp | 6 +++---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--\n> 4 files changed, 7 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 0357b2a630f7..841c7f4bef0f 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -57,7 +57,7 @@ public:\n> \t\t\t\t const Size &size) const;\n> \tint setFormat(V4L2SubdeviceFormat *format);\n> \n> -\tconst ControlInfoMap &controls() const;\n> +\tconst ControlInfoMap &subdevControls() const;\n> \tControlList getControls(const std::vector &ids);\n> \tint setControls(ControlList *ctrls);\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 18dc6d723e27..337d73c524bf 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -617,10 +617,10 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> }\n> \n> /**\n> - * \\brief Retrieve the supported V4L2 controls and their information\n> - * \\return A map of the V4L2 controls supported by the sensor\n> + * \\brief Retrieve the controls supported by the V4L2 subdev and their information\n> + * \\return A map of the V4L2 controls supported by the video subdevice\n> */\n> -const ControlInfoMap &CameraSensor::controls() const\n> +const ControlInfoMap &CameraSensor::subdevControls() const\n> {\n> \treturn subdev_->controls();\n> }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 5ce37febc1d7..628d1f39bfbd 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -906,7 +906,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t}\n> \n> \tstd::map entityControls;\n> -\tentityControls.emplace(0, data->sensor_->controls());\n> +\tentityControls.emplace(0, data->sensor_->subdevControls());\n> \n> \tIPAOperationData ipaConfig;\n> \tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 36325ffbbd7d..d5f6e8a453ff 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -338,7 +338,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> \n> int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> {\n> -\tControlList controls(data->sensor_->controls());\n> +\tControlList controls(data->sensor_->subdevControls());\n> \n> \tfor (auto it : request->controls()) {\n> \t\tunsigned int id = it.first;\n> @@ -480,7 +480,7 @@ int VimcCameraData::init()\n> \t\treturn -ENODEV;\n> \n> \t/* Initialise the supported controls. */\n> -\tconst ControlInfoMap &controls = sensor_->controls();\n> +\tconst ControlInfoMap &controls = sensor_->subdevControls();\n> \tControlInfoMap::Map ctrls;\n> \n> \tfor (const auto &ctrl : controls) {\n> -- \n> 2.29.2\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 C4631C0F1A\n\tfor ;\n\tTue, 29 Dec 2020 17:10:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B600615B2;\n\tTue, 29 Dec 2020 18:10:13 +0100 (CET)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B23DD6031F\n\tfor ;\n\tTue, 29 Dec 2020 18:10:11 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id x20so32150629lfe.12\n\tfor ;\n\tTue, 29 Dec 2020 09:10:11 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tg8sm6732595ljk.66.2020.12.29.09.10.09\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 29 Dec 2020 09:10:09 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Neal9xJw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=qVg7inJwb1slYYZTH5+PKvK+wpyoveUYk3lBm2eks+8=;\n\tb=Neal9xJwv7uw5J+U+1YZ/LbTEvNdV3BwO90BSo6NeFJ4NBuqMb8yM18xgT2DzdZ228\n\t78wpDD/mrdJDuqjH7IYJrp9rFYgJPPXLa+ajWL8/R5ijI9NcDOVPsr7JB5C6N67FQcwl\n\tSKfmWPOXjQMnivNMsDNvJof5eO60xJIldMKDJjXxndSNH/njRU1RH5VRX521+u8pqpWJ\n\tSYJ1Tp3+zyntCpAAGsVcZu0OwqnPqcTg1OpZWuigCkhhZI9Ab2GOJJCOVMhSXYAuFjJG\n\tZa63k1+KcHYG4bRofRElAl1TBlQEAuBLW7jYcsCJwZrbQT7CsBnOnRQesma8FcgXD4JS\n\tYEDw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=qVg7inJwb1slYYZTH5+PKvK+wpyoveUYk3lBm2eks+8=;\n\tb=ACrhNWWf6BmnMdgPJrbSNjaXx7S00yYLrJEx4T1JlJ0SQpwGDqWKxF4dJIhOgWsVE/\n\tm1sJwUmtFHE1MnuuDoX0pkhlSa03LXMMEEr8UEgLi72ydjBgeFGK6bk0Ij0pTYJjwV60\n\tb7iTIHDfevfTcHNzkDRBQ+N9gTq4FqWjsgkm4ma/iWJK0rc2yeFF9tVH24xqrf6yZ6/Z\n\tQJey5ay/5xhL3tbz11TTdPUB0Ebd41lDleRBlcVUA+MDc8B1h7/1WHsYpgFaaalZStwz\n\tuvXzt3B4sKy4CHp8wrH0ECb+LLRfMynvogE+NLusP1bj8Ds7F+RuD5ELcaEA/Mv9755v\n\tlW/w==","X-Gm-Message-State":"AOAM531XFd53Tstyn8msI0sT8fZ+59EJEpoJ8EBoo/ogarx35gHSY1o4\n\txJE0hXac8UlDRwYHoiDO/Du1Hq3fM5YfXg==","X-Google-Smtp-Source":"ABdhPJw/YSibLjfIQ33auf/bpCp2c54hXhd9p7ZVKlk2lgj4dEa0XTg0owgiCcrMZJVXFsdvXWuXsg==","X-Received":"by 2002:a19:8116:: with SMTP id\n\tc22mr22768999lfd.211.1609261811078; \n\tTue, 29 Dec 2020 09:10:11 -0800 (PST)","Date":"Tue, 29 Dec 2020 18:10:08 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201228165600.53987-1-jacopo@jmondi.org>\n\t<20201228165600.53987-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201228165600.53987-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: camera_sensor:\n\tRename controls() method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14410,"web_url":"https://patchwork.libcamera.org/comment/14410/","msgid":"<20201230094708.yvkfthdy3dhksbs5@uno.localdomain>","date":"2020-12-30T09:47:08","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: camera_sensor:\n\tRename controls() method","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Dec 29, 2020 at 06:10:08PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-12-28 17:55:58 +0100, Jacopo Mondi wrote:\n> > The CameraSensor::controls() methods returns the information relative to\n> > the V4L2 controls registered by the sensor sub-device driver. Its\n> > current use is to inform the IPA module of two pipelines (RkISP1 and\n> > VIMC) about the V4L2 controls limits.\n> >\n> > The CameraSensor class has a controls_ field, which is instead the\n> > ControlInfoMap of libcamera controls registered by the CameraSensor\n> > class and meant to be exported as Camera controls.\n> >\n> > To prepare to register libcamera controls in the CameraSensor::controls_\n> > info map, and remove any ambiguity on the intended usage of\n> > CameraSensor::controls(), rename the method in\n> > CameraSensor::subdevControls() and update its users in the code base.\n> >\n> > Reviewed-by: Paul Elder \n> > Signed-off-by: Jacopo Mondi \n>\n> The patch in itself is good but I'm not sure I understand the direction\n> we are taking with CameraSensor. I understand we should not expose any\n> V4L2 controls to applications but from the pipeline to core helpers (and\n> IPA) do we really wish to use libcamera controls? I can't see that we\n> will have core helpers or IPA that would work with both V4L2 and\n> something new. In my view such interfaces should be kept as close to the\n> drivers it interacts with, in this case V4L2.\n\nThis patch does not question that. The class has a map of libcamera\ncontrol info, created by inspecting the v4l2 controls of the subdev or\nthe sensor database. These should be libcamera cotrols, otherwise each\npipeline handler should make the translation itself, which is not\ncorrect in my opinion. The field is called controls_ and it seems\nnatural to retrieve it through a controls() method.\n\nIf we need to provide the IPA the raw V4L2 controls limits (and I\nthink we should keep doing so) they can be accessed with the here\nre-named subdevControls() method.\n\nIt's just about naming, really.\n\n>\n> I might be missing some use-case where mixing libcamera and V4L2\n> controls in helpers and lowlevel interfaces are desired but I can't\n> really see it. At first I thought maybe we would have some controls that\n> simply could be passed from application to CameraSensor but for all\n> cases I can think of I can see the possibility for an IPA to be involved\n> and then I get confused as we mix and match between the two already too\n> much in my taste ;-)\n\nIt's more about the fact the CameraSensor has to register\nlibcamera::ControlInfoMap created collecting information from the\nsensor db, the v4l2 kernel interface (and possibily other methods).\n\nThe ControlInfoMap should be made available to pipeline handlers, so\nthat they can use its content and, in addition to the\npipeline-specific controls (as it happens in the last patch of the\nseries) register the libcamera::Camera ControlInfoMap.\n\nAlternatively, each pipeline should make the translation between v4l2\ncontrols and libcamera controls, inspect the sensor db etc etc.\n\n>\n> Maybe the right solution is to move even further in the direction taken\n> here with the goal to not expose V4L2 controls outside the V4L2 classes.\n> In either case I would like to understand what we aim for.\n\nI think the IPA-pipeline interface should keep using v4l2 controls, at\nleast for now.\n\nThis patch only renames the method used to access them from the\nCameraSensor.\n\n>\n> For this rename change alone,\n>\n> Reviewed-by: Niklas Söderlund \n>\n> > ---\n> > include/libcamera/internal/camera_sensor.h | 2 +-\n> > src/libcamera/camera_sensor.cpp | 6 +++---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--\n> > 4 files changed, 7 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index 0357b2a630f7..841c7f4bef0f 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -57,7 +57,7 @@ public:\n> > \t\t\t\t const Size &size) const;\n> > \tint setFormat(V4L2SubdeviceFormat *format);\n> >\n> > -\tconst ControlInfoMap &controls() const;\n> > +\tconst ControlInfoMap &subdevControls() const;\n> > \tControlList getControls(const std::vector &ids);\n> > \tint setControls(ControlList *ctrls);\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 18dc6d723e27..337d73c524bf 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -617,10 +617,10 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> > }\n> >\n> > /**\n> > - * \\brief Retrieve the supported V4L2 controls and their information\n> > - * \\return A map of the V4L2 controls supported by the sensor\n> > + * \\brief Retrieve the controls supported by the V4L2 subdev and their information\n> > + * \\return A map of the V4L2 controls supported by the video subdevice\n> > */\n> > -const ControlInfoMap &CameraSensor::controls() const\n> > +const ControlInfoMap &CameraSensor::subdevControls() const\n> > {\n> > \treturn subdev_->controls();\n> > }\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 5ce37febc1d7..628d1f39bfbd 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -906,7 +906,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> > \t}\n> >\n> > \tstd::map entityControls;\n> > -\tentityControls.emplace(0, data->sensor_->controls());\n> > +\tentityControls.emplace(0, data->sensor_->subdevControls());\n> >\n> > \tIPAOperationData ipaConfig;\n> > \tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 36325ffbbd7d..d5f6e8a453ff 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -338,7 +338,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> >\n> > int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> > {\n> > -\tControlList controls(data->sensor_->controls());\n> > +\tControlList controls(data->sensor_->subdevControls());\n> >\n> > \tfor (auto it : request->controls()) {\n> > \t\tunsigned int id = it.first;\n> > @@ -480,7 +480,7 @@ int VimcCameraData::init()\n> > \t\treturn -ENODEV;\n> >\n> > \t/* Initialise the supported controls. */\n> > -\tconst ControlInfoMap &controls = sensor_->controls();\n> > +\tconst ControlInfoMap &controls = sensor_->subdevControls();\n> > \tControlInfoMap::Map ctrls;\n> >\n> > \tfor (const auto &ctrl : controls) {\n> > --\n> > 2.29.2\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 AE77BC0F1C\n\tfor ;\n\tWed, 30 Dec 2020 09:46:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4499A6031C;\n\tWed, 30 Dec 2020 10:46:56 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1DBD36031B\n\tfor ;\n\tWed, 30 Dec 2020 10:46:55 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 8D2B3E0007;\n\tWed, 30 Dec 2020 09:46:54 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 30 Dec 2020 10:47:08 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201230094708.yvkfthdy3dhksbs5@uno.localdomain>","References":"<20201228165600.53987-1-jacopo@jmondi.org>\n\t<20201228165600.53987-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: camera_sensor:\n\tRename controls() method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14415,"web_url":"https://patchwork.libcamera.org/comment/14415/","msgid":"","date":"2020-12-30T12:29:27","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: camera_sensor:\n\tRename controls() method","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Dec 30, 2020 at 10:47:08AM +0100, Jacopo Mondi wrote:\n> On Tue, Dec 29, 2020 at 06:10:08PM +0100, Niklas Söderlund wrote:\n> > On 2020-12-28 17:55:58 +0100, Jacopo Mondi wrote:\n> > > The CameraSensor::controls() methods returns the information relative to\n> > > the V4L2 controls registered by the sensor sub-device driver. Its\n> > > current use is to inform the IPA module of two pipelines (RkISP1 and\n> > > VIMC) about the V4L2 controls limits.\n> > >\n> > > The CameraSensor class has a controls_ field, which is instead the\n> > > ControlInfoMap of libcamera controls registered by the CameraSensor\n> > > class and meant to be exported as Camera controls.\n> > >\n> > > To prepare to register libcamera controls in the CameraSensor::controls_\n> > > info map, and remove any ambiguity on the intended usage of\n> > > CameraSensor::controls(), rename the method in\n> > > CameraSensor::subdevControls() and update its users in the code base.\n> > >\n> > > Reviewed-by: Paul Elder \n> > > Signed-off-by: Jacopo Mondi \n> >\n> > The patch in itself is good but I'm not sure I understand the direction\n> > we are taking with CameraSensor. I understand we should not expose any\n> > V4L2 controls to applications but from the pipeline to core helpers (and\n> > IPA) do we really wish to use libcamera controls? I can't see that we\n> > will have core helpers or IPA that would work with both V4L2 and\n> > something new. In my view such interfaces should be kept as close to the\n> > drivers it interacts with, in this case V4L2.\n> \n> This patch does not question that. The class has a map of libcamera\n> control info, created by inspecting the v4l2 controls of the subdev or\n> the sensor database. These should be libcamera cotrols, otherwise each\n> pipeline handler should make the translation itself, which is not\n> correct in my opinion. The field is called controls_ and it seems\n> natural to retrieve it through a controls() method.\n\nI disagree here. The camera sensor is internal, the controls it exposes\nmust not be copied verbatim by the pipeline handler to the controls\nexposed by the camera class. I'm OK using libcamera::controls right now\nto avoid growing this series, but we'll need to switch to\nsensor-specific controls sooner than latter. The exposure time is a good\nexample, we will possibly need to exposure it as a coarse exposure time\nexpressed as a number of lines and a fine exposure time expressed as a\nnumber of pixels, instead of a single exposure time, due to how\nill-defined the latter is, especially with the existing V4L2 controls\nrelated to the exposure time (EXPOSURE, HBLANK, VBLANK, PIXEL_RATE and\nof course the active analog crop rectangle - see my review of previous\npatches in the series). I'm not sure what will be needed exactly, but\nthe CameraSensor class should certainly not expose anything directly to\nthe public API.\n\n> If we need to provide the IPA the raw V4L2 controls limits (and I\n> think we should keep doing so) they can be accessed with the here\n> re-named subdevControls() method.\n\nI don't think we should provide V4L2 controls to the IPA. We should\nprovide a single set of sensor-specific controls, that will provide a\ngood abstraction of how a camera sensor operates, without being tied to\na particular kernel API (V4L2 in this case). We can have both controls()\nand subdevControls() in the interim, but that's really buying technical\ndebt, which is growing quite large and worries me. We need to at least\nbe on the same page regarding the direction we want to take. Could you\nplease add appropriate \\todo comments ?\n\n> It's just about naming, really.\n> \n> > I might be missing some use-case where mixing libcamera and V4L2\n> > controls in helpers and lowlevel interfaces are desired but I can't\n> > really see it. At first I thought maybe we would have some controls that\n> > simply could be passed from application to CameraSensor but for all\n> > cases I can think of I can see the possibility for an IPA to be involved\n> > and then I get confused as we mix and match between the two already too\n> > much in my taste ;-)\n> \n> It's more about the fact the CameraSensor has to register\n> libcamera::ControlInfoMap created collecting information from the\n> sensor db, the v4l2 kernel interface (and possibily other methods).\n> \n> The ControlInfoMap should be made available to pipeline handlers, so\n> that they can use its content and, in addition to the\n> pipeline-specific controls (as it happens in the last patch of the\n> series) register the libcamera::Camera ControlInfoMap.\n> \n> Alternatively, each pipeline should make the translation between v4l2\n> controls and libcamera controls, inspect the sensor db etc etc.\n\nPipeline handlers should translate between sensor-specific controls and\nlibcamera public controls.\n\n> > Maybe the right solution is to move even further in the direction taken\n> > here with the goal to not expose V4L2 controls outside the V4L2 classes.\n> > In either case I would like to understand what we aim for.\n> \n> I think the IPA-pipeline interface should keep using v4l2 controls, at\n> least for now.\n\nFor now as in this patch series, yes. This should however be as short\nterm as possible, it's quite urgent to fix this.\n\n> This patch only renames the method used to access them from the\n> CameraSensor.\n> \n> > For this rename change alone,\n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 2 +-\n> > > src/libcamera/camera_sensor.cpp | 6 +++---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--\n> > > 4 files changed, 7 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index 0357b2a630f7..841c7f4bef0f 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -57,7 +57,7 @@ public:\n> > > \t\t\t\t const Size &size) const;\n> > > \tint setFormat(V4L2SubdeviceFormat *format);\n> > >\n> > > -\tconst ControlInfoMap &controls() const;\n> > > +\tconst ControlInfoMap &subdevControls() const;\n> > > \tControlList getControls(const std::vector &ids);\n> > > \tint setControls(ControlList *ctrls);\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 18dc6d723e27..337d73c524bf 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -617,10 +617,10 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> > > }\n> > >\n> > > /**\n> > > - * \\brief Retrieve the supported V4L2 controls and their information\n> > > - * \\return A map of the V4L2 controls supported by the sensor\n> > > + * \\brief Retrieve the controls supported by the V4L2 subdev and their information\n> > > + * \\return A map of the V4L2 controls supported by the video subdevice\n> > > */\n> > > -const ControlInfoMap &CameraSensor::controls() const\n> > > +const ControlInfoMap &CameraSensor::subdevControls() const\n> > > {\n> > > \treturn subdev_->controls();\n> > > }\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 5ce37febc1d7..628d1f39bfbd 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -906,7 +906,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> > > \t}\n> > >\n> > > \tstd::map entityControls;\n> > > -\tentityControls.emplace(0, data->sensor_->controls());\n> > > +\tentityControls.emplace(0, data->sensor_->subdevControls());\n> > >\n> > > \tIPAOperationData ipaConfig;\n> > > \tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > index 36325ffbbd7d..d5f6e8a453ff 100644\n> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > @@ -338,7 +338,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> > >\n> > > int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> > > {\n> > > -\tControlList controls(data->sensor_->controls());\n> > > +\tControlList controls(data->sensor_->subdevControls());\n> > >\n> > > \tfor (auto it : request->controls()) {\n> > > \t\tunsigned int id = it.first;\n> > > @@ -480,7 +480,7 @@ int VimcCameraData::init()\n> > > \t\treturn -ENODEV;\n> > >\n> > > \t/* Initialise the supported controls. */\n> > > -\tconst ControlInfoMap &controls = sensor_->controls();\n> > > +\tconst ControlInfoMap &controls = sensor_->subdevControls();\n> > > \tControlInfoMap::Map ctrls;\n> > >\n> > > \tfor (const auto &ctrl : controls) {","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 CCEE9C0F1C\n\tfor ;\n\tWed, 30 Dec 2020 12:29:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63122615B2;\n\tWed, 30 Dec 2020 13:29:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E67B560320\n\tfor ;\n\tWed, 30 Dec 2020 13:29:40 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC5B42A3;\n\tWed, 30 Dec 2020 13:29:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"A+5t9ph9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609331380;\n\tbh=IowPe4zaj4noWmLF9eLlpi+MzaJSF0rAPIa9/zEMABw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A+5t9ph93NOp78fr6bgVyrE9O4sIDjvmQRAY4dDzyqdFCcLMZjGtMXqlbJrBaBQX6\n\tppdWVTvBMmoP8LmWuxK+0pqINxwG3ShcVxOHtJkfz/NdceQ3aYGt126iy3bIAYCbI5\n\tZrjuA1Y7EnEMvIUNSY2hV1VH8aINhujk5bCLd7j8=","Date":"Wed, 30 Dec 2020 14:29:27 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201228165600.53987-1-jacopo@jmondi.org>\n\t<20201228165600.53987-4-jacopo@jmondi.org>\n\t\n\t<20201230094708.yvkfthdy3dhksbs5@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201230094708.yvkfthdy3dhksbs5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: camera_sensor:\n\tRename controls() method","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]