[{"id":17691,"web_url":"https://patchwork.libcamera.org/comment/17691/","msgid":"<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>","date":"2021-06-22T10:31:02","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> Provides a function to set the camera sensor a test pattern mode.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> 2 files changed, 40 insertions(+)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index e133ebf4..8b9f84c9 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -43,6 +43,7 @@ public:\n> \t{\n> \t\treturn testPatternModes_;\n> \t}\n> +\tint setTestPatternMode(uint8_t testPatternMode);\n>\n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 70bbd97a..ce8ff274 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> * \\return The list of test pattern modes\n> */\n>\n> +/**\n> + * \\brief Set the camera sensor a specified controls::TestPatternMode\n\nDoxygen is puzzling me recently... the testPatternMode paramter is not\ndocumented but I don't get a warning :/\n\nAlso, I'm a bit debated about where to actually place this function\n(or better, it's initTestPatternModes which is misplaced)\nif we have to respect the declaration order or the way you sorted them\nhere (which is more logical) is better.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> +{\n> +\tif (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> +\t\t testPatternMode) == testPatternModes_.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> +\t\t\t\t\t << testPatternMode;\n> +\t\treturn -EINVAL;\n> +\t}\n\nDo we need this ? The testPatternModes_ map is constructed using the\ncamera sensor properties, the below find() on the props->testPatternModes\nmap should gurantee that is supported. Also, if a test pattern mode is\nnot reported as part of testPatternModes_ applications shouldn't set\nit...\n\n> +\n> +\tconst CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> +\tif (!props)\n> +\t\treturn -EINVAL;\n> +\n> +\tauto it = props->testPatternModes.find(testPatternMode);\n> +\tASSERT(it != props->testPatternModes.end());\n\nYeah, I think the assertion here is more than enough..\n\n> +\tconst uint8_t index = it->second;\n> +\n> +\tControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> +\tif (ctrls.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Failed to retrieve test pattern control\";\n> +\t\treturn -EINVAL;\n> +\t}\n\nWe won't register the TestPatternMode control in first place the V4L2\ncontrol is not supported..\n\n> +\n> +\tControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> +\tif (value.get() == index)\n> +\t\treturn 0;\n\nThe V4L2 control framework should take care of this by itself, I think\nwe can set the control regardless\n\n> +\n> +\tvalue.set(index);\n> +\tctrls.set(V4L2_CID_TEST_PATTERN, value);\n> +\n> +\treturn setControls(&ctrls);\n> +}\n> +\n> /**\n> * \\brief Retrieve the best sensor format for a desired output\n> * \\param[in] mbusCodes The list of acceptable media bus codes\n> --\n> 2.32.0.288.g62a8d224e6-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 DBD48C321A\n\tfor ;\n\tTue, 22 Jun 2021 10:30:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C06768932;\n\tTue, 22 Jun 2021 12:30:15 +0200 (CEST)","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 D51F060292\n\tfor ;\n\tTue, 22 Jun 2021 12:30:14 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 3A491E0003;\n\tTue, 22 Jun 2021 10:30:13 +0000 (UTC)"],"Date":"Tue, 22 Jun 2021 12:31:02 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210622023654.969162-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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\" "}},{"id":17709,"web_url":"https://patchwork.libcamera.org/comment/17709/","msgid":"","date":"2021-06-23T07:49:46","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for reviewing.\n\nOn Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > Provides a function to set the camera sensor a test pattern mode.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > 2 files changed, 40 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index e133ebf4..8b9f84c9 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -43,6 +43,7 @@ public:\n> > {\n> > return testPatternModes_;\n> > }\n> > + int setTestPatternMode(uint8_t testPatternMode);\n> >\n> > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > const Size &size) const;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 70bbd97a..ce8ff274 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > * \\return The list of test pattern modes\n> > */\n> >\n> > +/**\n> > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n>\n> Doxygen is puzzling me recently... the testPatternMode paramter is not\n> documented but I don't get a warning :/\n>\n> Also, I'm a bit debated about where to actually place this function\n> (or better, it's initTestPatternModes which is misplaced)\n> if we have to respect the declaration order or the way you sorted them\n> here (which is more logical) is better.\n>\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > +{\n> > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > + testPatternMode) == testPatternModes_.end()) {\n> > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > + << testPatternMode;\n> > + return -EINVAL;\n> > + }\n>\n> Do we need this ? The testPatternModes_ map is constructed using the\n> camera sensor properties, the below find() on the props->testPatternModes\n> map should gurantee that is supported. Also, if a test pattern mode is\n> not reported as part of testPatternModes_ applications shouldn't set\n> it...\n>\n> > +\n> > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > + if (!props)\n> > + return -EINVAL;\n> > +\n> > + auto it = props->testPatternModes.find(testPatternMode);\n> > + ASSERT(it != props->testPatternModes.end());\n>\n> Yeah, I think the assertion here is more than enough..\n>\n> > + const uint8_t index = it->second;\n> > +\n> > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > + if (ctrls.empty()) {\n> > + LOG(CameraSensor, Error)\n> > + << \"Failed to retrieve test pattern control\";\n> > + return -EINVAL;\n> > + }\n>\n> We won't register the TestPatternMode control in first place the V4L2\n> control is not supported..\n\nReplace this with ASSERT.\n\n>\n> > +\n> > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > + if (value.get() == index)\n> > + return 0;\n>\n> The V4L2 control framework should take care of this by itself, I think\n> we can set the control regardless\n>\n\nYes, I put this in order to save a redundant Ioctl call.\nDo you want to remove that?\n\n-Hiro\n> > +\n> > + value.set(index);\n> > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > +\n> > + return setControls(&ctrls);\n> > +}\n> > +\n> > /**\n> > * \\brief Retrieve the best sensor format for a desired output\n> > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > --\n> > 2.32.0.288.g62a8d224e6-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 41B1CC321A\n\tfor ;\n\tWed, 23 Jun 2021 07:49:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5FAE68935;\n\tWed, 23 Jun 2021 09:49:57 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64DEC68931\n\tfor ;\n\tWed, 23 Jun 2021 09:49:56 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id my49so2544207ejc.7\n\tfor ;\n\tWed, 23 Jun 2021 00:49:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Ut2HRtIT\"; 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=3ziggknjvbFv/gPu6/rGvfKqZQwL1GwFxtL5h0L5xWg=;\n\tb=Ut2HRtITioSXebrrw8XIzz7HHv+8/9SNV7QV+6n3Flinhly1cOPxDayS8/k4ZgTZse\n\tTSEjhCwiMTMs6rB+fz/vo0Iqaf0XZwXJPL5iGfx8Rs3Lit6OLZ2plg2mdJjN9l5FEyrB\n\tQNB2g10ic3hhyCxAjz4oIeiWd4O06YcOC0ZmA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=3ziggknjvbFv/gPu6/rGvfKqZQwL1GwFxtL5h0L5xWg=;\n\tb=l9yaKoytYmTpu0EcnXbxKDDWGEapEyiFHBec/HGzMIqfqMHUloOq+XuXHCwyxGfi7x\n\ty5LAmuhPR1xpECsKDrV2AmaRK7XaIXGR8rG0LVJFbtrNJ1F/9kBSuGd/JfDJeLXUgAuX\n\t/hVi+/gEHKuWXO9DGu3B8sP5nG6Q17uSqOT08ZYP6VFf8WMJE7xLvV5gvg6QJCMz1H/k\n\tWH57PRBSS74GLh3NSozU06146hi3rYUYxEfhnuDj/4+WkBSPTPkkyxy9p12ZQ3I2rZnG\n\t/3f1b9i77sIdp9uO3WnkLxZA83pCl3PlJSmHd7Ox/X+hvDnGaKPqvV0iuwriHpXjanMT\n\tfzwQ==","X-Gm-Message-State":"AOAM531pbT1LZl+tfv63G6MCbCECGqQzy6EK6Dgz4WqVjSxAp+QhdeX+\n\tSruSN1bmDzW8GAoy92I3kQTjjJNf3o897ewelQIAIA==","X-Google-Smtp-Source":"ABdhPJx8iNvkfnuR/l+QX/X/UGiRgfjiIBuzAukUfuFic//ZblrEHAn2IQutSqoQI8hfgQ8uQE8FDhFtuPXF99lEwz8=","X-Received":"by 2002:a17:907:20da:: with SMTP id\n\tqq26mr8390920ejb.42.1624434595980; \n\tWed, 23 Jun 2021 00:49:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>","In-Reply-To":"<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 23 Jun 2021 16:49:46 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17713,"web_url":"https://patchwork.libcamera.org/comment/17713/","msgid":"<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>","date":"2021-06-23T08:07:04","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for reviewing.\n>\n> On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > Provides a function to set the camera sensor a test pattern mode.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > 2 files changed, 40 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index e133ebf4..8b9f84c9 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -43,6 +43,7 @@ public:\n> > > {\n> > > return testPatternModes_;\n> > > }\n> > > + int setTestPatternMode(uint8_t testPatternMode);\n> > >\n> > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > const Size &size) const;\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 70bbd97a..ce8ff274 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > * \\return The list of test pattern modes\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> >\n> > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > documented but I don't get a warning :/\n> >\n> > Also, I'm a bit debated about where to actually place this function\n> > (or better, it's initTestPatternModes which is misplaced)\n> > if we have to respect the declaration order or the way you sorted them\n> > here (which is more logical) is better.\n> >\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > +{\n> > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > + testPatternMode) == testPatternModes_.end()) {\n> > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > + << testPatternMode;\n> > > + return -EINVAL;\n> > > + }\n> >\n> > Do we need this ? The testPatternModes_ map is constructed using the\n> > camera sensor properties, the below find() on the props->testPatternModes\n> > map should gurantee that is supported. Also, if a test pattern mode is\n> > not reported as part of testPatternModes_ applications shouldn't set\n> > it...\n> >\n> > > +\n> > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > + if (!props)\n> > > + return -EINVAL;\n> > > +\n> > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > + ASSERT(it != props->testPatternModes.end());\n> >\n> > Yeah, I think the assertion here is more than enough..\n> >\n> > > + const uint8_t index = it->second;\n> > > +\n> > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > + if (ctrls.empty()) {\n> > > + LOG(CameraSensor, Error)\n> > > + << \"Failed to retrieve test pattern control\";\n> > > + return -EINVAL;\n> > > + }\n> >\n> > We won't register the TestPatternMode control in first place the V4L2\n> > control is not supported..\n>\n> Replace this with ASSERT.\n>\n> >\n> > > +\n> > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > + if (value.get() == index)\n> > > + return 0;\n> >\n> > The V4L2 control framework should take care of this by itself, I think\n> > we can set the control regardless\n> >\n>\n> Yes, I put this in order to save a redundant Ioctl call.\n> Do you want to remove that?\n\nWell, the ctrls ControlList you create with getControls() goes through\nan IOCTL :) We're talking details, but if we want to retain the\nstate of the testPatterMode we can make it a class member, so that the\ncode can be reduced to (not tested pseudo-code)\n\n{\n if (testPatternMode == testPatternMode_)\n return 0;\n\n const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n if (!props)\n return -EINVAL;\n\n auto it = props->testPatternModes.find(testPatternMode);\n ASSERT(it != props->testPatternModes.end());\n\n ControlList ctrls(controls());\n ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n\n int ret = setControls(&ctrls);\n if (ret)\n return ret;\n\n testPatternMode_ = testPatternMode;\n\n return 0;\n}\n\nI also wonder if the ASSERT() is not too harsh and we shouldn't just\nerror out. If the application tries to set an unsupported test pattern,\nthe whole library would crash, maybe we should be a little more\nindulgent.\n\nThanks\n j\n\n>\n> -Hiro\n> > > +\n> > > + value.set(index);\n> > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > +\n> > > + return setControls(&ctrls);\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Retrieve the best sensor format for a desired output\n> > > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > > --\n> > > 2.32.0.288.g62a8d224e6-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 7C6C5C321B\n\tfor ;\n\tWed, 23 Jun 2021 08:06:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CE606893B;\n\tWed, 23 Jun 2021 10:06:17 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A10C768931\n\tfor ;\n\tWed, 23 Jun 2021 10:06:15 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 04DF520000C;\n\tWed, 23 Jun 2021 08:06:14 +0000 (UTC)"],"Date":"Wed, 23 Jun 2021 10:07:04 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17718,"web_url":"https://patchwork.libcamera.org/comment/17718/","msgid":"","date":"2021-06-23T08:27:02","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Wed, Jun 23, 2021 at 5:06 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for reviewing.\n> >\n> > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > > Provides a function to set the camera sensor a test pattern mode.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > > 2 files changed, 40 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > index e133ebf4..8b9f84c9 100644\n> > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > @@ -43,6 +43,7 @@ public:\n> > > > {\n> > > > return testPatternModes_;\n> > > > }\n> > > > + int setTestPatternMode(uint8_t testPatternMode);\n> > > >\n> > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > const Size &size) const;\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index 70bbd97a..ce8ff274 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > > * \\return The list of test pattern modes\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> > >\n> > > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > > documented but I don't get a warning :/\n> > >\n> > > Also, I'm a bit debated about where to actually place this function\n> > > (or better, it's initTestPatternModes which is misplaced)\n> > > if we have to respect the declaration order or the way you sorted them\n> > > here (which is more logical) is better.\n> > >\n> > > > + *\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > > +{\n> > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > + testPatternMode) == testPatternModes_.end()) {\n> > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > > + << testPatternMode;\n> > > > + return -EINVAL;\n> > > > + }\n> > >\n> > > Do we need this ? The testPatternModes_ map is constructed using the\n> > > camera sensor properties, the below find() on the props->testPatternModes\n> > > map should gurantee that is supported. Also, if a test pattern mode is\n> > > not reported as part of testPatternModes_ applications shouldn't set\n> > > it...\n> > >\n> > > > +\n> > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > + if (!props)\n> > > > + return -EINVAL;\n> > > > +\n> > > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > > + ASSERT(it != props->testPatternModes.end());\n> > >\n> > > Yeah, I think the assertion here is more than enough..\n> > >\n> > > > + const uint8_t index = it->second;\n> > > > +\n> > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > > + if (ctrls.empty()) {\n> > > > + LOG(CameraSensor, Error)\n> > > > + << \"Failed to retrieve test pattern control\";\n> > > > + return -EINVAL;\n> > > > + }\n> > >\n> > > We won't register the TestPatternMode control in first place the V4L2\n> > > control is not supported..\n> >\n> > Replace this with ASSERT.\n> >\n> > >\n> > > > +\n> > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > > + if (value.get() == index)\n> > > > + return 0;\n> > >\n> > > The V4L2 control framework should take care of this by itself, I think\n> > > we can set the control regardless\n> > >\n> >\n> > Yes, I put this in order to save a redundant Ioctl call.\n> > Do you want to remove that?\n>\n> Well, the ctrls ControlList you create with getControls() goes through\n> an IOCTL :) We're talking details, but if we want to retain the\n> state of the testPatterMode we can make it a class member, so that the\n> code can be reduced to (not tested pseudo-code)\n>\n> {\n> if (testPatternMode == testPatternMode_)\n> return 0;\n>\n> const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> if (!props)\n> return -EINVAL;\n>\n> auto it = props->testPatternModes.find(testPatternMode);\n> ASSERT(it != props->testPatternModes.end());\n>\n> ControlList ctrls(controls());\n> ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n>\n> int ret = setControls(&ctrls);\n> if (ret)\n> return ret;\n>\n> testPatternMode_ = testPatternMode;\n>\n> return 0;\n> }\n>\n> I also wonder if the ASSERT() is not too harsh and we shouldn't just\n> error out. If the application tries to set an unsupported test pattern,\n> the whole library would crash, maybe we should be a little more\n> indulgent.\n>\n\nI see what you meant. Thanks!\n\n> Thanks\n> j\n>\n> >\n> > -Hiro\n> > > > +\n> > > > + value.set(index);\n> > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > > +\n> > > > + return setControls(&ctrls);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > > > --\n> > > > 2.32.0.288.g62a8d224e6-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 C209CC321A\n\tfor ;\n\tWed, 23 Jun 2021 08:27:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 203DF68938;\n\tWed, 23 Jun 2021 10:27:14 +0200 (CEST)","from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com\n\t[IPv6:2a00:1450:4864:20::62e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE26768931\n\tfor ;\n\tWed, 23 Jun 2021 10:27:12 +0200 (CEST)","by mail-ej1-x62e.google.com with SMTP id bg14so2677928ejb.9\n\tfor ;\n\tWed, 23 Jun 2021 01:27:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"WEiFXMGJ\"; 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=lG3S65nrLBHDpCmz2pqXtC0t8szHBzl6ziC4uo6CF1g=;\n\tb=WEiFXMGJR3OEdFn61oz1dRm8fP1C6eekgNbj8R4fQzUItai0h/nCRjYnOD2mbL6I9G\n\t+35oVXmowYot39iOR++kkxG9iOoLGRDzuuYiK15NLD5HhJD9B4E/frGnXOVBU2RkYpGH\n\tZu5e+++KhguCBFyYVzyXo4P994zO4aozVZhh0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=lG3S65nrLBHDpCmz2pqXtC0t8szHBzl6ziC4uo6CF1g=;\n\tb=jzMiD/QHQ9C6L2GlP4mHHGIADf7An2IIUmwk8oTvq+BIue+4aYsA1XmtdEm24ndZ6a\n\tp7C7MCDLzeQoj/F3+XESlJWxPQVhpeuT2gVXXkBk2kJ6FIRfiOCzznDjBTeNixupEdBX\n\to6UbNAre6PwqeySm4AxJNwp79Pusp2vg+IviMPGZBScGgLyOuzBFb1xYXaVVqAcXKZUX\n\t1MtSrZKix73Emnx8yP2cHqVJorcI4rHuxAR1lErkvGMjzWNt/vKJMb56xPm0vuahPjtB\n\tul1V1DfaLB0L5PEm7kDOA0F9wCSVZS4rNrjJwXOO0TtzyxOkCy8maZZ68IrABe6kEQtl\n\t+37A==","X-Gm-Message-State":"AOAM5309UMgX7feLzlU663ZR0FZ+6MGUKth6Nog0jscTkjSMLLTHwmOm\n\t6/fM6p2PUIrwGRANcq7QRqCMIHUzoguddMIHPy9VuA==","X-Google-Smtp-Source":"ABdhPJxHecUpPZ1MADsEH6hA88ASvx3+oXognugPvAb2TB52mhshDhrhqAv8bv8vPVQ2emrjB9UFkbw59GudReZLWqQ=","X-Received":"by 2002:a17:906:a20b:: with SMTP id\n\tr11mr8651187ejy.221.1624436832558; \n\tWed, 23 Jun 2021 01:27:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t\n\t<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>","In-Reply-To":"<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 23 Jun 2021 17:27:02 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17842,"web_url":"https://patchwork.libcamera.org/comment/17842/","msgid":"","date":"2021-06-28T03:12:32","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:\n> On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > > Provides a function to set the camera sensor a test pattern mode.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > > 2 files changed, 40 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > index e133ebf4..8b9f84c9 100644\n> > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > @@ -43,6 +43,7 @@ public:\n> > > > {\n> > > > return testPatternModes_;\n> > > > }\n> > > > + int setTestPatternMode(uint8_t testPatternMode);\n> > > >\n> > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > const Size &size) const;\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index 70bbd97a..ce8ff274 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > > * \\return The list of test pattern modes\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> > >\n> > > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > > documented but I don't get a warning :/\n> > >\n> > > Also, I'm a bit debated about where to actually place this function\n> > > (or better, it's initTestPatternModes which is misplaced)\n> > > if we have to respect the declaration order or the way you sorted them\n> > > here (which is more logical) is better.\n> > >\n> > > > + *\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > > +{\n> > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > + testPatternMode) == testPatternModes_.end()) {\n> > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > > + << testPatternMode;\n> > > > + return -EINVAL;\n> > > > + }\n> > >\n> > > Do we need this ? The testPatternModes_ map is constructed using the\n> > > camera sensor properties, the below find() on the props->testPatternModes\n> > > map should gurantee that is supported. Also, if a test pattern mode is\n> > > not reported as part of testPatternModes_ applications shouldn't set\n> > > it...\n> > >\n> > > > +\n> > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > + if (!props)\n> > > > + return -EINVAL;\n\nWe already retrieve this in CameraSensor::initStaticProperties(), let's\ncache the pointer in the CameraSensor class.\n\n> > > > +\n> > > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > > + ASSERT(it != props->testPatternModes.end());\n> > >\n> > > Yeah, I think the assertion here is more than enough..\n> > >\n> > > > + const uint8_t index = it->second;\n> > > > +\n> > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > > + if (ctrls.empty()) {\n> > > > + LOG(CameraSensor, Error)\n> > > > + << \"Failed to retrieve test pattern control\";\n> > > > + return -EINVAL;\n> > > > + }\n> > >\n> > > We won't register the TestPatternMode control in first place the V4L2\n> > > control is not supported..\n> >\n> > Replace this with ASSERT.\n> >\n> > > > +\n> > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > > + if (value.get() == index)\n> > > > + return 0;\n> > >\n> > > The V4L2 control framework should take care of this by itself, I think\n> > > we can set the control regardless\n> >\n> > Yes, I put this in order to save a redundant Ioctl call.\n> > Do you want to remove that?\n> \n> Well, the ctrls ControlList you create with getControls() goes through\n> an IOCTL :) We're talking details, but if we want to retain the\n> state of the testPatterMode we can make it a class member, so that the\n> code can be reduced to (not tested pseudo-code)\n> \n> {\n> if (testPatternMode == testPatternMode_)\n> return 0;\n> \n> const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> if (!props)\n> return -EINVAL;\n> \n> auto it = props->testPatternModes.find(testPatternMode);\n> ASSERT(it != props->testPatternModes.end());\n> \n> ControlList ctrls(controls());\n> ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> \n> int ret = setControls(&ctrls);\n> if (ret)\n> return ret;\n> \n> testPatternMode_ = testPatternMode;\n> \n> return 0;\n> }\n> \n> I also wonder if the ASSERT() is not too harsh and we shouldn't just\n> error out. If the application tries to set an unsupported test pattern,\n> the whole library would crash, maybe we should be a little more\n> indulgent.\n\nYes, not crashing is a good idea :-)\n\n> > > > +\n> > > > + value.set(index);\n> > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > > +\n> > > > + return setControls(&ctrls);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > * \\param[in] mbusCodes The list of acceptable media bus codes","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 800B7C321A\n\tfor ;\n\tMon, 28 Jun 2021 03:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3F47684D5;\n\tMon, 28 Jun 2021 05:12:36 +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 A100D6028C\n\tfor ;\n\tMon, 28 Jun 2021 05:12:34 +0200 (CEST)","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 10C9D57E;\n\tMon, 28 Jun 2021 05:12:33 +0200 (CEST)"],"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=\"MMJtKiCT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624849954;\n\tbh=cZXUJIMAnFJl8JJt0SRxiee3oi36EI+K82dHV1H/Q0E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MMJtKiCTpGn8iPw1FduTOobb85cqu1pn5udxgieRgKcmDMYJ0nSEAOmsPLLHLf3RT\n\tBCepe5zV9Ag4yH17ukGGqYHZTf4YcVWmx96OkND/jhdqXqaGWegz9VIRGFJEyqhdiF\n\tfttjQbI3gatxBi6/J1p9F1PMepbc9FXXNb3v0uyY=","Date":"Mon, 28 Jun 2021 06:12:32 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t\n\t<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17843,"web_url":"https://patchwork.libcamera.org/comment/17843/","msgid":"","date":"2021-06-28T03:28:22","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> Thank you for the patch.\n\nThe patch is from Hiro of course :-)\n\nAnother comment, do we need such a ad-hoc function, or could we handle\nthe test pattern mode using a more generic interface, with a ControlList\n? The CameraSensor::setControls() takes V4L2 controls today, which makes\nit unsuitable for this purpose. I would ideally like to see all this\nfixed, but I suppose it's a too big task for this series. It adds up to\nthe technical debt though, so there's a high chance that I'll push back\nmore strongly on the next patch that adds a ad-hoc function to\nCameraSensor, until we reach a point where the push back will be a\nstrong nack and all this will need to be refactored by an unlucky\nperson. All this means that nobody should think that delaying the\nhandling of technical debt is a smart move to move it to someone else's\nplate, it may come back around :-)\n\n> On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:\n> > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > > > Provides a function to set the camera sensor a test pattern mode.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > > > 2 files changed, 40 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > > index e133ebf4..8b9f84c9 100644\n> > > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > > @@ -43,6 +43,7 @@ public:\n> > > > > {\n> > > > > return testPatternModes_;\n> > > > > }\n> > > > > + int setTestPatternMode(uint8_t testPatternMode);\n> > > > >\n> > > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > > const Size &size) const;\n> > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > index 70bbd97a..ce8ff274 100644\n> > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > > > * \\return The list of test pattern modes\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> > > >\n> > > > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > > > documented but I don't get a warning :/\n> > > >\n> > > > Also, I'm a bit debated about where to actually place this function\n> > > > (or better, it's initTestPatternModes which is misplaced)\n> > > > if we have to respect the declaration order or the way you sorted them\n> > > > here (which is more logical) is better.\n> > > >\n> > > > > + *\n> > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > + */\n> > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > > > +{\n> > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > > + testPatternMode) == testPatternModes_.end()) {\n> > > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > > > + << testPatternMode;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > >\n> > > > Do we need this ? The testPatternModes_ map is constructed using the\n> > > > camera sensor properties, the below find() on the props->testPatternModes\n> > > > map should gurantee that is supported. Also, if a test pattern mode is\n> > > > not reported as part of testPatternModes_ applications shouldn't set\n> > > > it...\n> > > >\n> > > > > +\n> > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > > + if (!props)\n> > > > > + return -EINVAL;\n> \n> We already retrieve this in CameraSensor::initStaticProperties(), let's\n> cache the pointer in the CameraSensor class.\n> \n> > > > > +\n> > > > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > > > + ASSERT(it != props->testPatternModes.end());\n> > > >\n> > > > Yeah, I think the assertion here is more than enough..\n> > > >\n> > > > > + const uint8_t index = it->second;\n> > > > > +\n> > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > > > + if (ctrls.empty()) {\n> > > > > + LOG(CameraSensor, Error)\n> > > > > + << \"Failed to retrieve test pattern control\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > >\n> > > > We won't register the TestPatternMode control in first place the V4L2\n> > > > control is not supported..\n> > >\n> > > Replace this with ASSERT.\n> > >\n> > > > > +\n> > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > > > + if (value.get() == index)\n> > > > > + return 0;\n> > > >\n> > > > The V4L2 control framework should take care of this by itself, I think\n> > > > we can set the control regardless\n> > >\n> > > Yes, I put this in order to save a redundant Ioctl call.\n> > > Do you want to remove that?\n> > \n> > Well, the ctrls ControlList you create with getControls() goes through\n> > an IOCTL :) We're talking details, but if we want to retain the\n> > state of the testPatterMode we can make it a class member, so that the\n> > code can be reduced to (not tested pseudo-code)\n> > \n> > {\n> > if (testPatternMode == testPatternMode_)\n> > return 0;\n> > \n> > const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > if (!props)\n> > return -EINVAL;\n> > \n> > auto it = props->testPatternModes.find(testPatternMode);\n> > ASSERT(it != props->testPatternModes.end());\n> > \n> > ControlList ctrls(controls());\n> > ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> > \n> > int ret = setControls(&ctrls);\n> > if (ret)\n> > return ret;\n> > \n> > testPatternMode_ = testPatternMode;\n> > \n> > return 0;\n> > }\n> > \n> > I also wonder if the ASSERT() is not too harsh and we shouldn't just\n> > error out. If the application tries to set an unsupported test pattern,\n> > the whole library would crash, maybe we should be a little more\n> > indulgent.\n> \n> Yes, not crashing is a good idea :-)\n> \n> > > > > +\n> > > > > + value.set(index);\n> > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > > > +\n> > > > > + return setControls(&ctrls);\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > > * \\param[in] mbusCodes The list of acceptable media bus codes","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 2E6EAC321D\n\tfor ;\n\tMon, 28 Jun 2021 03:28:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B184684D5;\n\tMon, 28 Jun 2021 05:28: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 C55C26028C\n\tfor ;\n\tMon, 28 Jun 2021 05:28:23 +0200 (CEST)","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 3C4A957E;\n\tMon, 28 Jun 2021 05:28:23 +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=\"MaEfBp9K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624850903;\n\tbh=8MyV0X1F3nUhFi999NKVb6St3TYS4nqN8fIKNEqEGtQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MaEfBp9KfO7YbSsxAQO9FUvz2Ons1zg4fGSoiWhh2j+74muZtke1s89+BU7OY2uTf\n\tnfvshYPaUL9QD/rGhPjW/fi9X3HtG7ULlgPV2Ar73u2OXKvadmU1BBUD6GgTb6xtmy\n\tZiIn9dK4kD2ACDW7UHEopFfi66t4thMoklBqkvco=","Date":"Mon, 28 Jun 2021 06:28:22 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t\n\t<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17860,"web_url":"https://patchwork.libcamera.org/comment/17860/","msgid":"","date":"2021-06-28T05:16:00","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart\n wrote:\n>\n> On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n>\n> The patch is from Hiro of course :-)\n>\n> Another comment, do we need such a ad-hoc function, or could we handle\n> the test pattern mode using a more generic interface, with a ControlList\n> ? The CameraSensor::setControls() takes V4L2 controls today, which makes\n> it unsuitable for this purpose. I would ideally like to see all this\n> fixed, but I suppose it's a too big task for this series. It adds up to\n> the technical debt though, so there's a high chance that I'll push back\n> more strongly on the next patch that adds a ad-hoc function to\n> CameraSensor, until we reach a point where the push back will be a\n> strong nack and all this will need to be refactored by an unlucky\n> person. All this means that nobody should think that delaying the\n> handling of technical debt is a smart move to move it to someone else's\n> plate, it may come back around :-)\n>\n\nIf we want to use CameraSensor::setControls(), it is necessary for a\npipeline handler to know the v4l2 index associated with a specific\ntest pattern mode.\nHow can we do that? Is it okay for a pipeline handler to read test\npattern mode in camera properties?\n\n> > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:\n> > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > > > > Provides a function to set the camera sensor a test pattern mode.\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > ---\n> > > > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > > > > 2 files changed, 40 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > > > index e133ebf4..8b9f84c9 100644\n> > > > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > > > @@ -43,6 +43,7 @@ public:\n> > > > > > {\n> > > > > > return testPatternModes_;\n> > > > > > }\n> > > > > > + int setTestPatternMode(uint8_t testPatternMode);\n> > > > > >\n> > > > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > > > const Size &size) const;\n> > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > > index 70bbd97a..ce8ff274 100644\n> > > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > > > > * \\return The list of test pattern modes\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> > > > >\n> > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > > > > documented but I don't get a warning :/\n> > > > >\n> > > > > Also, I'm a bit debated about where to actually place this function\n> > > > > (or better, it's initTestPatternModes which is misplaced)\n> > > > > if we have to respect the declaration order or the way you sorted them\n> > > > > here (which is more logical) is better.\n> > > > >\n> > > > > > + *\n> > > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > > + */\n> > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > > > > +{\n> > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > > > + testPatternMode) == testPatternModes_.end()) {\n> > > > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > > > > + << testPatternMode;\n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > > > >\n> > > > > Do we need this ? The testPatternModes_ map is constructed using the\n> > > > > camera sensor properties, the below find() on the props->testPatternModes\n> > > > > map should gurantee that is supported. Also, if a test pattern mode is\n> > > > > not reported as part of testPatternModes_ applications shouldn't set\n> > > > > it...\n> > > > >\n> > > > > > +\n> > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > > > + if (!props)\n> > > > > > + return -EINVAL;\n> >\n> > We already retrieve this in CameraSensor::initStaticProperties(), let's\n> > cache the pointer in the CameraSensor class.\n> >\n> > > > > > +\n> > > > > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > > > > + ASSERT(it != props->testPatternModes.end());\n> > > > >\n> > > > > Yeah, I think the assertion here is more than enough..\n> > > > >\n> > > > > > + const uint8_t index = it->second;\n> > > > > > +\n> > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > > > > + if (ctrls.empty()) {\n> > > > > > + LOG(CameraSensor, Error)\n> > > > > > + << \"Failed to retrieve test pattern control\";\n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > > > >\n> > > > > We won't register the TestPatternMode control in first place the V4L2\n> > > > > control is not supported..\n> > > >\n> > > > Replace this with ASSERT.\n> > > >\n> > > > > > +\n> > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > > > > + if (value.get() == index)\n> > > > > > + return 0;\n> > > > >\n> > > > > The V4L2 control framework should take care of this by itself, I think\n> > > > > we can set the control regardless\n> > > >\n> > > > Yes, I put this in order to save a redundant Ioctl call.\n> > > > Do you want to remove that?\n> > >\n> > > Well, the ctrls ControlList you create with getControls() goes through\n> > > an IOCTL :) We're talking details, but if we want to retain the\n> > > state of the testPatterMode we can make it a class member, so that the\n> > > code can be reduced to (not tested pseudo-code)\n> > >\n> > > {\n> > > if (testPatternMode == testPatternMode_)\n> > > return 0;\n> > >\n> > > const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > if (!props)\n> > > return -EINVAL;\n> > >\n> > > auto it = props->testPatternModes.find(testPatternMode);\n> > > ASSERT(it != props->testPatternModes.end());\n> > >\n> > > ControlList ctrls(controls());\n> > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> > >\n> > > int ret = setControls(&ctrls);\n> > > if (ret)\n> > > return ret;\n> > >\n> > > testPatternMode_ = testPatternMode;\n> > >\n> > > return 0;\n> > > }\n> > >\n> > > I also wonder if the ASSERT() is not too harsh and we shouldn't just\n> > > error out. If the application tries to set an unsupported test pattern,\n> > > the whole library would crash, maybe we should be a little more\n> > > indulgent.\n> >\n> > Yes, not crashing is a good idea :-)\n> >\n> > > > > > +\n> > > > > > + value.set(index);\n> > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > > > > +\n> > > > > > + return setControls(&ctrls);\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > > > * \\param[in] mbusCodes The list of acceptable media bus codes\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 02722C321E\n\tfor ;\n\tMon, 28 Jun 2021 05:16:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F7B7684D3;\n\tMon, 28 Jun 2021 07:16:14 +0200 (CEST)","from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com\n\t[IPv6:2a00:1450:4864:20::62e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81BE86028C\n\tfor ;\n\tMon, 28 Jun 2021 07:16:12 +0200 (CEST)","by mail-ej1-x62e.google.com with SMTP id gn32so27760753ejc.2\n\tfor ;\n\tSun, 27 Jun 2021 22:16:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"FYf6Offa\"; 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=9g2N9UQzIUGC7ywgxtuew/3J+iPd6X+WheoD+bhnDLE=;\n\tb=FYf6OffaG6Oj6d+rlD1xuTQC9+t+a1zEC/8qWqXnFln6edVcB5wnTw6WV+AjgYEt1g\n\tf8vZi6AHqiCkDIdsxl0zlxnijNPKoSIYt1KwyF0CBHLfaBFw28PaGpFNnK6UONGVZFZy\n\tEI4PL5VatmAwDdLMdCwi86Ejjue9kgfLSWkYU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=9g2N9UQzIUGC7ywgxtuew/3J+iPd6X+WheoD+bhnDLE=;\n\tb=Dch77j7xG/k7Hgrjl8bB2wXzDeBbRkqncIY1osuIHlmWWk8o8G136wHdK/5XgMv6GM\n\tK7jzmK8dVhpxmDwzKCAtIIwbLeReOHqjDQVa8I/HDymZ6RCeRJsYJSwqztLVd3kgdkr/\n\t5tnvaDcxd7sBk7JryfGKLbFqntYBeXz3itNXorOgkHzM0j5Sc8cx3y5sftixlUMfKJbH\n\tFxpNvn47m1N0Pq8ke2a0ssRoB/DiXKCb/43yAJLb30UUKl+MESygFKqa+PdqFAA3JthB\n\tFralFFDa2Dfw1uXcIr14BPS/quMjiTAsjj5YsLTEr7/3NW8MdwW8m4Xc8WFZivRRo6UP\n\tyRfA==","X-Gm-Message-State":"AOAM531lLvj/4Zx5cOcACgN8m0Da/Xy97Gl1EgkKWB5f+f3NrAvFj0Nu\n\t57DmjPCrRakjNHkQQpi8vsXOoTzuXrf6xNqSucqxow==","X-Google-Smtp-Source":"ABdhPJwJG2mw+/ZQQUXWTohzjg0TL7PzyphnhotNX77g6z3rEGHYzIPtv6r2sWG1vP6hZuCWq/QEOirvrR2ruBUsGok=","X-Received":"by 2002:a17:907:20da:: with SMTP id\n\tqq26mr21932172ejb.42.1624857372106; \n\tSun, 27 Jun 2021 22:16:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t\n\t<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 28 Jun 2021 14:16:00 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17861,"web_url":"https://patchwork.libcamera.org/comment/17861/","msgid":"","date":"2021-06-28T05:26:27","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Jun 28, 2021 at 02:16:00PM +0900, Hirokazu Honda wrote:\n> On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote:\n> > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the patch.\n> >\n> > The patch is from Hiro of course :-)\n> >\n> > Another comment, do we need such a ad-hoc function, or could we handle\n> > the test pattern mode using a more generic interface, with a ControlList\n> > ? The CameraSensor::setControls() takes V4L2 controls today, which makes\n> > it unsuitable for this purpose. I would ideally like to see all this\n> > fixed, but I suppose it's a too big task for this series. It adds up to\n> > the technical debt though, so there's a high chance that I'll push back\n> > more strongly on the next patch that adds a ad-hoc function to\n> > CameraSensor, until we reach a point where the push back will be a\n> > strong nack and all this will need to be refactored by an unlucky\n> > person. All this means that nobody should think that delaying the\n> > handling of technical debt is a smart move to move it to someone else's\n> > plate, it may come back around :-)\n> \n> If we want to use CameraSensor::setControls(), it is necessary for a\n> pipeline handler to know the v4l2 index associated with a specific\n> test pattern mode.\n> How can we do that? Is it okay for a pipeline handler to read test\n> pattern mode in camera properties?\n\nI'd prefer going the other way around and avoid exposing V4L2 controls\nfrom CameraSensor. I haven't really thought about how to do so yet\nthough.\n\n> > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:\n> > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > > > > > Provides a function to set the camera sensor a test pattern mode.\n> > > > > > >\n> > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > ---\n> > > > > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > > > > > 2 files changed, 40 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > > > > index e133ebf4..8b9f84c9 100644\n> > > > > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > > > > @@ -43,6 +43,7 @@ public:\n> > > > > > > {\n> > > > > > > return testPatternModes_;\n> > > > > > > }\n> > > > > > > + int setTestPatternMode(uint8_t testPatternMode);\n> > > > > > >\n> > > > > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > > > > const Size &size) const;\n> > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > > > index 70bbd97a..ce8ff274 100644\n> > > > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > > > > > * \\return The list of test pattern modes\n> > > > > > > */\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> > > > > >\n> > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > > > > > documented but I don't get a warning :/\n> > > > > >\n> > > > > > Also, I'm a bit debated about where to actually place this function\n> > > > > > (or better, it's initTestPatternModes which is misplaced)\n> > > > > > if we have to respect the declaration order or the way you sorted them\n> > > > > > here (which is more logical) is better.\n> > > > > >\n> > > > > > > + *\n> > > > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > > > + */\n> > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > > > > > +{\n> > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > > > > + testPatternMode) == testPatternModes_.end()) {\n> > > > > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > > > > > + << testPatternMode;\n> > > > > > > + return -EINVAL;\n> > > > > > > + }\n> > > > > >\n> > > > > > Do we need this ? The testPatternModes_ map is constructed using the\n> > > > > > camera sensor properties, the below find() on the props->testPatternModes\n> > > > > > map should gurantee that is supported. Also, if a test pattern mode is\n> > > > > > not reported as part of testPatternModes_ applications shouldn't set\n> > > > > > it...\n> > > > > >\n> > > > > > > +\n> > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > > > > + if (!props)\n> > > > > > > + return -EINVAL;\n> > >\n> > > We already retrieve this in CameraSensor::initStaticProperties(), let's\n> > > cache the pointer in the CameraSensor class.\n> > >\n> > > > > > > +\n> > > > > > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > > > > > + ASSERT(it != props->testPatternModes.end());\n> > > > > >\n> > > > > > Yeah, I think the assertion here is more than enough..\n> > > > > >\n> > > > > > > + const uint8_t index = it->second;\n> > > > > > > +\n> > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > > > > > + if (ctrls.empty()) {\n> > > > > > > + LOG(CameraSensor, Error)\n> > > > > > > + << \"Failed to retrieve test pattern control\";\n> > > > > > > + return -EINVAL;\n> > > > > > > + }\n> > > > > >\n> > > > > > We won't register the TestPatternMode control in first place the V4L2\n> > > > > > control is not supported..\n> > > > >\n> > > > > Replace this with ASSERT.\n> > > > >\n> > > > > > > +\n> > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > > > > > + if (value.get() == index)\n> > > > > > > + return 0;\n> > > > > >\n> > > > > > The V4L2 control framework should take care of this by itself, I think\n> > > > > > we can set the control regardless\n> > > > >\n> > > > > Yes, I put this in order to save a redundant Ioctl call.\n> > > > > Do you want to remove that?\n> > > >\n> > > > Well, the ctrls ControlList you create with getControls() goes through\n> > > > an IOCTL :) We're talking details, but if we want to retain the\n> > > > state of the testPatterMode we can make it a class member, so that the\n> > > > code can be reduced to (not tested pseudo-code)\n> > > >\n> > > > {\n> > > > if (testPatternMode == testPatternMode_)\n> > > > return 0;\n> > > >\n> > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > if (!props)\n> > > > return -EINVAL;\n> > > >\n> > > > auto it = props->testPatternModes.find(testPatternMode);\n> > > > ASSERT(it != props->testPatternModes.end());\n> > > >\n> > > > ControlList ctrls(controls());\n> > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> > > >\n> > > > int ret = setControls(&ctrls);\n> > > > if (ret)\n> > > > return ret;\n> > > >\n> > > > testPatternMode_ = testPatternMode;\n> > > >\n> > > > return 0;\n> > > > }\n> > > >\n> > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just\n> > > > error out. If the application tries to set an unsupported test pattern,\n> > > > the whole library would crash, maybe we should be a little more\n> > > > indulgent.\n> > >\n> > > Yes, not crashing is a good idea :-)\n> > >\n> > > > > > > +\n> > > > > > > + value.set(index);\n> > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > > > > > +\n> > > > > > > + return setControls(&ctrls);\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > /**\n> > > > > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > > > > * \\param[in] mbusCodes The list of acceptable media bus codes","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 3CF81C321A\n\tfor ;\n\tMon, 28 Jun 2021 05:26:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 953AF684D7;\n\tMon, 28 Jun 2021 07:26:30 +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 67E886028C\n\tfor ;\n\tMon, 28 Jun 2021 07:26:29 +0200 (CEST)","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 D3A35B8A;\n\tMon, 28 Jun 2021 07:26:28 +0200 (CEST)"],"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=\"eGnXHuzw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624857989;\n\tbh=u2Nphv4Ja1NOB1Q9ezcl0q5d91wSmz4XW9DQUYTHVsk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eGnXHuzwA82vDHfwUvZoyWRZ68hZoOC96sbjBHDLTrnwQANPPo7WfxrcNo3wVDFn6\n\t9YgHw30iqXfDAbA4BQV/p87ERFK8/af4uMEJ8cOTSDAZPQ+3H6/yDIK2Dko4yZrXYs\n\tRdkbQ7w2EctrnDtdGo/08gzr0yckcc5Z2MmH3O9M=","Date":"Mon, 28 Jun 2021 08:26:27 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t\n\t<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17863,"web_url":"https://patchwork.libcamera.org/comment/17863/","msgid":"","date":"2021-06-28T05:33:56","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 28, 2021 at 2:26 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Jun 28, 2021 at 02:16:00PM +0900, Hirokazu Honda wrote:\n> > On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote:\n> > > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for the patch.\n> > >\n> > > The patch is from Hiro of course :-)\n> > >\n> > > Another comment, do we need such a ad-hoc function, or could we handle\n> > > the test pattern mode using a more generic interface, with a ControlList\n> > > ? The CameraSensor::setControls() takes V4L2 controls today, which makes\n> > > it unsuitable for this purpose. I would ideally like to see all this\n> > > fixed, but I suppose it's a too big task for this series. It adds up to\n> > > the technical debt though, so there's a high chance that I'll push back\n> > > more strongly on the next patch that adds a ad-hoc function to\n> > > CameraSensor, until we reach a point where the push back will be a\n> > > strong nack and all this will need to be refactored by an unlucky\n> > > person. All this means that nobody should think that delaying the\n> > > handling of technical debt is a smart move to move it to someone else's\n> > > plate, it may come back around :-)\n> >\n> > If we want to use CameraSensor::setControls(), it is necessary for a\n> > pipeline handler to know the v4l2 index associated with a specific\n> > test pattern mode.\n> > How can we do that? Is it okay for a pipeline handler to read test\n> > pattern mode in camera properties?\n>\n> I'd prefer going the other way around and avoid exposing V4L2 controls\n> from CameraSensor. I haven't really thought about how to do so yet\n> though.\n>\n\nHmm, so how shall I do in this patch series?\n\n> > > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:\n> > > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> > > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> > > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > > > > > > Provides a function to set the camera sensor a test pattern mode.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > ---\n> > > > > > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > > > > > > 2 files changed, 40 insertions(+)\n> > > > > > > >\n> > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > > > > > index e133ebf4..8b9f84c9 100644\n> > > > > > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > > > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > > > > > @@ -43,6 +43,7 @@ public:\n> > > > > > > > {\n> > > > > > > > return testPatternModes_;\n> > > > > > > > }\n> > > > > > > > + int setTestPatternMode(uint8_t testPatternMode);\n> > > > > > > >\n> > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > > > > > const Size &size) const;\n> > > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > > > > index 70bbd97a..ce8ff274 100644\n> > > > > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > > > > > > * \\return The list of test pattern modes\n> > > > > > > > */\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> > > > > > >\n> > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > > > > > > documented but I don't get a warning :/\n> > > > > > >\n> > > > > > > Also, I'm a bit debated about where to actually place this function\n> > > > > > > (or better, it's initTestPatternModes which is misplaced)\n> > > > > > > if we have to respect the declaration order or the way you sorted them\n> > > > > > > here (which is more logical) is better.\n> > > > > > >\n> > > > > > > > + *\n> > > > > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > > > > + */\n> > > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > > > > > > +{\n> > > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > > > > > + testPatternMode) == testPatternModes_.end()) {\n> > > > > > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > > > > > > + << testPatternMode;\n> > > > > > > > + return -EINVAL;\n> > > > > > > > + }\n> > > > > > >\n> > > > > > > Do we need this ? The testPatternModes_ map is constructed using the\n> > > > > > > camera sensor properties, the below find() on the props->testPatternModes\n> > > > > > > map should gurantee that is supported. Also, if a test pattern mode is\n> > > > > > > not reported as part of testPatternModes_ applications shouldn't set\n> > > > > > > it...\n> > > > > > >\n> > > > > > > > +\n> > > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > > > > > + if (!props)\n> > > > > > > > + return -EINVAL;\n> > > >\n> > > > We already retrieve this in CameraSensor::initStaticProperties(), let's\n> > > > cache the pointer in the CameraSensor class.\n> > > >\n> > > > > > > > +\n> > > > > > > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > > > > > > + ASSERT(it != props->testPatternModes.end());\n> > > > > > >\n> > > > > > > Yeah, I think the assertion here is more than enough..\n> > > > > > >\n> > > > > > > > + const uint8_t index = it->second;\n> > > > > > > > +\n> > > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > > > > > > + if (ctrls.empty()) {\n> > > > > > > > + LOG(CameraSensor, Error)\n> > > > > > > > + << \"Failed to retrieve test pattern control\";\n> > > > > > > > + return -EINVAL;\n> > > > > > > > + }\n> > > > > > >\n> > > > > > > We won't register the TestPatternMode control in first place the V4L2\n> > > > > > > control is not supported..\n> > > > > >\n> > > > > > Replace this with ASSERT.\n> > > > > >\n> > > > > > > > +\n> > > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > > > > > > + if (value.get() == index)\n> > > > > > > > + return 0;\n> > > > > > >\n> > > > > > > The V4L2 control framework should take care of this by itself, I think\n> > > > > > > we can set the control regardless\n> > > > > >\n> > > > > > Yes, I put this in order to save a redundant Ioctl call.\n> > > > > > Do you want to remove that?\n> > > > >\n> > > > > Well, the ctrls ControlList you create with getControls() goes through\n> > > > > an IOCTL :) We're talking details, but if we want to retain the\n> > > > > state of the testPatterMode we can make it a class member, so that the\n> > > > > code can be reduced to (not tested pseudo-code)\n> > > > >\n> > > > > {\n> > > > > if (testPatternMode == testPatternMode_)\n> > > > > return 0;\n> > > > >\n> > > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > > if (!props)\n> > > > > return -EINVAL;\n> > > > >\n> > > > > auto it = props->testPatternModes.find(testPatternMode);\n> > > > > ASSERT(it != props->testPatternModes.end());\n> > > > >\n> > > > > ControlList ctrls(controls());\n> > > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> > > > >\n> > > > > int ret = setControls(&ctrls);\n> > > > > if (ret)\n> > > > > return ret;\n> > > > >\n> > > > > testPatternMode_ = testPatternMode;\n> > > > >\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just\n> > > > > error out. If the application tries to set an unsupported test pattern,\n> > > > > the whole library would crash, maybe we should be a little more\n> > > > > indulgent.\n> > > >\n> > > > Yes, not crashing is a good idea :-)\n> > > >\n> > > > > > > > +\n> > > > > > > > + value.set(index);\n> > > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > > > > > > +\n> > > > > > > > + return setControls(&ctrls);\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > /**\n> > > > > > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > > > > > * \\param[in] mbusCodes The list of acceptable media bus codes\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 1DD80C321E\n\tfor ;\n\tMon, 28 Jun 2021 05:34:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47B88684D5;\n\tMon, 28 Jun 2021 07:34:09 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45CB16028C\n\tfor ;\n\tMon, 28 Jun 2021 07:34:08 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id bg14so27735274ejb.9\n\tfor ;\n\tSun, 27 Jun 2021 22:34:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"moHttXRP\"; 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=hJO1fIyAj93QzaMlumHiqeTsxqIG3eSRKWyNnWm3hmo=;\n\tb=moHttXRP6JcnxDAnN5hVnNKLgypULzhUqWycwwGaHDDCYmMjbwrdc2dsYlQMLc+Q5D\n\tZ+iRk0+YmCcDL6tOo2m0GlgVvSAn5l+BjCAcHlNxeMZUGxqgLHowN6iYcNYMiRZsMlCC\n\t0Wgpt6N9YgSYzczuY6x0IyiZvOw4Q/1drKI2c=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=hJO1fIyAj93QzaMlumHiqeTsxqIG3eSRKWyNnWm3hmo=;\n\tb=J6Ehj39SgzPrFNk21MYWnzb/IEEfEoM2N0lTSfs4/2l7LGPGAKB3dyG9bU2/XtXhnx\n\tphYq3E1sI86zV8HJpfrCm9N6PicsGmOv4bkUq7bF+pm7LIulAKTlCZbaGUrQXTiLvcLA\n\ttYMdwnyCawMAx9nvqwe4uIC86+swfjAcK1qYxE9Rbi2I+yFjGdYdSLR3PPbA48r/epwf\n\t32zkk+NInDxZp+K39HwF3mh3sNrZlZgC9dYtzo81wjpmqR0IBNhUNHOlKM5Z/biD84DE\n\thA4DnDWcP+5K+9tFItbJli84rZcH1QoXGIU/5S3NNoSx+P/eeAfA/CJ9zZr1rfqXYkX7\n\tWq+g==","X-Gm-Message-State":"AOAM532O0mHJ7cBdVQBzLSuTJSyVRH3YuR107J+cl8WXk26aV3jXhDJZ\n\tMHpDV0/d4Ty5mdi6OZ0ZSA2YtGWR3kFQxgVIV7Na7A==","X-Google-Smtp-Source":"ABdhPJxcxC96USkQLPCK2r+z0j/LOKtPnEIQ0UYk8r7T8835xZo2lTls/0bJ7UKqQo+jVxHHISq82ORpJOAUlErYMlk=","X-Received":"by 2002:a17:907:20da:: with SMTP id\n\tqq26mr21987411ejb.42.1624858447918; \n\tSun, 27 Jun 2021 22:34:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t\n\t<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 28 Jun 2021 14:33:56 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17864,"web_url":"https://patchwork.libcamera.org/comment/17864/","msgid":"","date":"2021-06-28T05:45:30","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Jun 28, 2021 at 02:33:56PM +0900, Hirokazu Honda wrote:\n> On Mon, Jun 28, 2021 at 2:26 PM Laurent Pinchart wrote:\n> > On Mon, Jun 28, 2021 at 02:16:00PM +0900, Hirokazu Honda wrote:\n> > > On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote:\n> > > > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Thank you for the patch.\n> > > >\n> > > > The patch is from Hiro of course :-)\n> > > >\n> > > > Another comment, do we need such a ad-hoc function, or could we handle\n> > > > the test pattern mode using a more generic interface, with a ControlList\n> > > > ? The CameraSensor::setControls() takes V4L2 controls today, which makes\n> > > > it unsuitable for this purpose. I would ideally like to see all this\n> > > > fixed, but I suppose it's a too big task for this series. It adds up to\n> > > > the technical debt though, so there's a high chance that I'll push back\n> > > > more strongly on the next patch that adds a ad-hoc function to\n> > > > CameraSensor, until we reach a point where the push back will be a\n> > > > strong nack and all this will need to be refactored by an unlucky\n> > > > person. All this means that nobody should think that delaying the\n> > > > handling of technical debt is a smart move to move it to someone else's\n> > > > plate, it may come back around :-)\n> > >\n> > > If we want to use CameraSensor::setControls(), it is necessary for a\n> > > pipeline handler to know the v4l2 index associated with a specific\n> > > test pattern mode.\n> > > How can we do that? Is it okay for a pipeline handler to read test\n> > > pattern mode in camera properties?\n> >\n> > I'd prefer going the other way around and avoid exposing V4L2 controls\n> > from CameraSensor. I haven't really thought about how to do so yet\n> > though.\n> \n> Hmm, so how shall I do in this patch series?\n\nI think it may be too big for this series, so I'm ok adding the\nsetTestPatternMode() for now. We'll have to address this issue though,\nso if you think of a way this could be done, we can start brainstorming\nit.\n\n> > > > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:\n> > > > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:\n> > > > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:\n> > > > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:\n> > > > > > > > > Provides a function to set the camera sensor a test pattern mode.\n> > > > > > > > >\n> > > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > > ---\n> > > > > > > > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++\n> > > > > > > > > 2 files changed, 40 insertions(+)\n> > > > > > > > >\n> > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > > > > > > index e133ebf4..8b9f84c9 100644\n> > > > > > > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > > > > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > > > > > > @@ -43,6 +43,7 @@ public:\n> > > > > > > > > {\n> > > > > > > > > return testPatternModes_;\n> > > > > > > > > }\n> > > > > > > > > + int setTestPatternMode(uint8_t testPatternMode);\n> > > > > > > > >\n> > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > > > > > > const Size &size) const;\n> > > > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > > > > > index 70bbd97a..ce8ff274 100644\n> > > > > > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const\n> > > > > > > > > * \\return The list of test pattern modes\n> > > > > > > > > */\n> > > > > > > > >\n> > > > > > > > > +/**\n> > > > > > > > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> > > > > > > >\n> > > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not\n> > > > > > > > documented but I don't get a warning :/\n> > > > > > > >\n> > > > > > > > Also, I'm a bit debated about where to actually place this function\n> > > > > > > > (or better, it's initTestPatternModes which is misplaced)\n> > > > > > > > if we have to respect the declaration order or the way you sorted them\n> > > > > > > > here (which is more logical) is better.\n> > > > > > > >\n> > > > > > > > > + *\n> > > > > > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > > > > > + */\n> > > > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)\n> > > > > > > > > +{\n> > > > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > > > > > > + testPatternMode) == testPatternModes_.end()) {\n> > > > > > > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> > > > > > > > > + << testPatternMode;\n> > > > > > > > > + return -EINVAL;\n> > > > > > > > > + }\n> > > > > > > >\n> > > > > > > > Do we need this ? The testPatternModes_ map is constructed using the\n> > > > > > > > camera sensor properties, the below find() on the props->testPatternModes\n> > > > > > > > map should gurantee that is supported. Also, if a test pattern mode is\n> > > > > > > > not reported as part of testPatternModes_ applications shouldn't set\n> > > > > > > > it...\n> > > > > > > >\n> > > > > > > > > +\n> > > > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > > > > > > + if (!props)\n> > > > > > > > > + return -EINVAL;\n> > > > >\n> > > > > We already retrieve this in CameraSensor::initStaticProperties(), let's\n> > > > > cache the pointer in the CameraSensor class.\n> > > > >\n> > > > > > > > > +\n> > > > > > > > > + auto it = props->testPatternModes.find(testPatternMode);\n> > > > > > > > > + ASSERT(it != props->testPatternModes.end());\n> > > > > > > >\n> > > > > > > > Yeah, I think the assertion here is more than enough..\n> > > > > > > >\n> > > > > > > > > + const uint8_t index = it->second;\n> > > > > > > > > +\n> > > > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });\n> > > > > > > > > + if (ctrls.empty()) {\n> > > > > > > > > + LOG(CameraSensor, Error)\n> > > > > > > > > + << \"Failed to retrieve test pattern control\";\n> > > > > > > > > + return -EINVAL;\n> > > > > > > > > + }\n> > > > > > > >\n> > > > > > > > We won't register the TestPatternMode control in first place the V4L2\n> > > > > > > > control is not supported..\n> > > > > > >\n> > > > > > > Replace this with ASSERT.\n> > > > > > >\n> > > > > > > > > +\n> > > > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);\n> > > > > > > > > + if (value.get() == index)\n> > > > > > > > > + return 0;\n> > > > > > > >\n> > > > > > > > The V4L2 control framework should take care of this by itself, I think\n> > > > > > > > we can set the control regardless\n> > > > > > >\n> > > > > > > Yes, I put this in order to save a redundant Ioctl call.\n> > > > > > > Do you want to remove that?\n> > > > > >\n> > > > > > Well, the ctrls ControlList you create with getControls() goes through\n> > > > > > an IOCTL :) We're talking details, but if we want to retain the\n> > > > > > state of the testPatterMode we can make it a class member, so that the\n> > > > > > code can be reduced to (not tested pseudo-code)\n> > > > > >\n> > > > > > {\n> > > > > > if (testPatternMode == testPatternMode_)\n> > > > > > return 0;\n> > > > > >\n> > > > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > > > if (!props)\n> > > > > > return -EINVAL;\n> > > > > >\n> > > > > > auto it = props->testPatternModes.find(testPatternMode);\n> > > > > > ASSERT(it != props->testPatternModes.end());\n> > > > > >\n> > > > > > ControlList ctrls(controls());\n> > > > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> > > > > >\n> > > > > > int ret = setControls(&ctrls);\n> > > > > > if (ret)\n> > > > > > return ret;\n> > > > > >\n> > > > > > testPatternMode_ = testPatternMode;\n> > > > > >\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just\n> > > > > > error out. If the application tries to set an unsupported test pattern,\n> > > > > > the whole library would crash, maybe we should be a little more\n> > > > > > indulgent.\n> > > > >\n> > > > > Yes, not crashing is a good idea :-)\n> > > > >\n> > > > > > > > > +\n> > > > > > > > > + value.set(index);\n> > > > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value);\n> > > > > > > > > +\n> > > > > > > > > + return setControls(&ctrls);\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > /**\n> > > > > > > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > > > > > > * \\param[in] mbusCodes The list of acceptable media bus codes","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 A472AC321A\n\tfor ;\n\tMon, 28 Jun 2021 05:45:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18D3D684D7;\n\tMon, 28 Jun 2021 07:45:33 +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 4DFB26028C\n\tfor ;\n\tMon, 28 Jun 2021 07:45:32 +0200 (CEST)","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 AB251B8A;\n\tMon, 28 Jun 2021 07:45:31 +0200 (CEST)"],"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=\"vehDl+YO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624859131;\n\tbh=x9u+/o8Zw+QxN4G8Elue7IN3050t4C+O8c6CD0OnRoE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vehDl+YOCknU5CzgXHhUbxGM/M/KfNFnFzOGSiNPRyTx9ZMRm702OrAddPCbykbTI\n\tTHaNbxgOagKbYy3iaK4GAax+XOxmJ6SR6uBL5jUXld+7i/IwrRFBbVnrlyq44wXLLR\n\tbWR9UmSAwZIZ/EsbysiHFr/S1Ko2D0rgJkLslZ/E=","Date":"Mon, 28 Jun 2021 08:45:30 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210622023654.969162-1-hiroh@chromium.org>\n\t<20210622023654.969162-2-hiroh@chromium.org>\n\t<20210622103102.vnwx5cimkj2xddwl@uno.localdomain>\n\t\n\t<20210623080704.w6uhp5qr66ebhepb@uno.localdomain>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]