[{"id":17006,"web_url":"https://patchwork.libcamera.org/comment/17006/","msgid":"<20210518122237.yyvtkuyx55t7x2se@uno.localdomain>","date":"2021-05-18T12:22:37","subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera:\n\tCameraSensorProperties: Adds table of v4l2 index and test pattern","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro\n\nOn Thu, May 06, 2021 at 04:54:46PM +0900, Hirokazu Honda wrote:\n> In V4L2 API, a driver returns an index (also with a name) to\n> represent a test pattern, but it is a driver specific what test\n> pattern is represented by the index. Therefore, this adds a\n> mapping table from the index and a test pattern into a static\n> configuration of a sensor.\n\nA bit convoluted... What about something along the lines of:\n\nThe V4L2 specification defines the sensor test pattern modes\nthrough a menu control, where a numerical index is associated to\na string that describes the test pattern. The index-to-pattern\nmapping is driver specific and requires a corresponding representation\nin the library.\n\nAdd to the static list of CameraSensorProperties a map of indexes to\nlibcamera::controls::TestPatternModes values to be able to map the\nindexes returned by the driver to the corresponding test pattern mode.\n\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> .../internal/camera_sensor_properties.h | 2 ++\n> src/libcamera/camera_sensor_properties.cpp | 20 +++++++++++++++++++\n> 2 files changed, 22 insertions(+)\n>\n> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> index f5e242cb..f52890b4 100644\n> --- a/include/libcamera/internal/camera_sensor_properties.h\n> +++ b/include/libcamera/internal/camera_sensor_properties.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_SENSOR_CAMERA_SENSOR_PROPERTIES_H__\n> #define __LIBCAMERA_SENSOR_CAMERA_SENSOR_PROPERTIES_H__\n>\n> +#include \n> #include \n>\n> #include \n> @@ -17,6 +18,7 @@ struct CameraSensorProperties {\n> \tstatic const CameraSensorProperties *get(const std::string &sensor);\n>\n> \tSize unitCellSize;\n> +\tstd::map testPatternModeMap;\n\nI would expect that an uint8_t to uint8_t should do\n\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> index 6ded31dc..89eaf9f8 100644\n> --- a/src/libcamera/camera_sensor_properties.cpp\n> +++ b/src/libcamera/camera_sensor_properties.cpp\n> @@ -9,6 +9,8 @@\n>\n> #include \n>\n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n>\n> /**\n> @@ -44,15 +46,33 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> */\n> const CameraSensorProperties *CameraSensorProperties::get(const std::string &sensor)\n> {\n> +\n\nUnrelated blank line\n\n> \tstatic const std::map sensorProps = {\n> \t\t{ \"imx219\", {\n> \t\t\t.unitCellSize = { 1120, 1120 },\n> +\t\t\t.testPatternModeMap = {\n> +\t\t\t\t{ 0, controls::draft::TestPatternModeOff },\n> +\t\t\t\t{ 1, controls::draft::TestPatternModeColorBars },\n> +\t\t\t\t{ 2, controls::draft::TestPatternModeSolidColor },\n> +\t\t\t\t{ 3, controls::draft::TestPatternModeColorBarsFadeToGray },\n> +\t\t\t\t{ 4, controls::draft::TestPatternModePn9 },\n> +\t\t\t},\n\nThis is from the imx219 driver\n\n#define IMX219_TEST_PATTERN_DISABLE\t0\n#define IMX219_TEST_PATTERN_SOLID_COLOR\t1\n#define IMX219_TEST_PATTERN_COLOR_BARS\t2\n#define IMX219_TEST_PATTERN_GREY_COLOR\t3\n#define IMX219_TEST_PATTERN_PN9\t\t4\n\nShould you map 1 to controls::draft::TestPatternModeSolidColor and 2\nto controls::draft::TestPatternModeColorBars ?\n\n> \t\t} },\n> \t\t{ \"ov5670\", {\n> \t\t\t.unitCellSize = { 1120, 1120 },\n> +\t\t\t.testPatternModeMap = {\n> +\t\t\t\t{ 0, controls::draft::TestPatternModeOff },\n> +\t\t\t\t{ 1, controls::draft::TestPatternModeColorBars },\n> +\t\t\t},\n\nThe driver only defines:\n#define OV5670_TEST_PATTERN_ENABLE\tBIT(3)\n\ndoes this map to ColorBars ?\n\n> +\n\nUnrelated blank line\n\n> \t\t} },\n> \t\t{ \"ov13858\", {\n> \t\t\t.unitCellSize = { 1120, 1120 },\n> +\t\t\t.testPatternModeMap = {\n> +\t\t\t\t{ 0, controls::draft::TestPatternModeOff },\n> +\t\t\t\t{ 1, controls::draft::TestPatternModeColorBars },\n> +\t\t\t\t{ 2, controls::draft::TestPatternModeColorBarsFadeToGray },\n\nThe driver only reports 1 mode:\n#define OV13858_TEST_PATTERN_ENABLE\tBIT(7)\n\nWhere does the second come from ?\n\nThanks\n j\n\n> +\t\t\t},\n> \t\t} },\n> \t};\n>\n> --\n> 2.31.1.607.g51e8a6a459-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6A6EAC31FF\n\tfor ;\n\tTue, 18 May 2021 12:21:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB93868920;\n\tTue, 18 May 2021 14:21:54 +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 E0FAD68918\n\tfor ;\n\tTue, 18 May 2021 14:21:52 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 47028E0017;\n\tTue, 18 May 2021 12:21:51 +0000 (UTC)"],"Date":"Tue, 18 May 2021 14:22:37 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210518122237.yyvtkuyx55t7x2se@uno.localdomain>","References":"<20210506075449.1761752-1-hiroh@chromium.org>\n\t<20210506075449.1761752-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210506075449.1761752-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera:\n\tCameraSensorProperties: Adds table of v4l2 index and test pattern","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":17010,"web_url":"https://patchwork.libcamera.org/comment/17010/","msgid":"","date":"2021-05-19T05:31:23","subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera:\n\tCameraSensorProperties: Adds table of v4l2 index and test pattern","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thanks for reviewing.\n\nOn Tue, May 18, 2021 at 9:21 PM Jacopo Mondi wrote:\n\n> Hi Hiro\n>\n> On Thu, May 06, 2021 at 04:54:46PM +0900, Hirokazu Honda wrote:\n> > In V4L2 API, a driver returns an index (also with a name) to\n> > represent a test pattern, but it is a driver specific what test\n> > pattern is represented by the index. Therefore, this adds a\n> > mapping table from the index and a test pattern into a static\n> > configuration of a sensor.\n>\n> A bit convoluted... What about something along the lines of:\n>\n> The V4L2 specification defines the sensor test pattern modes\n> through a menu control, where a numerical index is associated to\n> a string that describes the test pattern. The index-to-pattern\n> mapping is driver specific and requires a corresponding representation\n> in the library.\n>\n> Add to the static list of CameraSensorProperties a map of indexes to\n> libcamera::controls::TestPatternModes values to be able to map the\n> indexes returned by the driver to the corresponding test pattern mode.\n>\n>\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > .../internal/camera_sensor_properties.h | 2 ++\n> > src/libcamera/camera_sensor_properties.cpp | 20 +++++++++++++++++++\n> > 2 files changed, 22 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor_properties.h\n> b/include/libcamera/internal/camera_sensor_properties.h\n> > index f5e242cb..f52890b4 100644\n> > --- a/include/libcamera/internal/camera_sensor_properties.h\n> > +++ b/include/libcamera/internal/camera_sensor_properties.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __LIBCAMERA_SENSOR_CAMERA_SENSOR_PROPERTIES_H__\n> > #define __LIBCAMERA_SENSOR_CAMERA_SENSOR_PROPERTIES_H__\n> >\n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -17,6 +18,7 @@ struct CameraSensorProperties {\n> > static const CameraSensorProperties *get(const std::string\n> &sensor);\n> >\n> > Size unitCellSize;\n> > + std::map testPatternModeMap;\n>\n> I would expect that an uint8_t to uint8_t should do\n>\n>\nIt should be enough although I do so because control value and index value\nare int32_t.\nI don't have a strong opinion. Changed to uint8_t.\n\n\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> b/src/libcamera/camera_sensor_properties.cpp\n> > index 6ded31dc..89eaf9f8 100644\n> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > @@ -9,6 +9,8 @@\n> >\n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/log.h\"\n> >\n> > /**\n> > @@ -44,15 +46,33 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> > */\n> > const CameraSensorProperties *CameraSensorProperties::get(const\n> std::string &sensor)\n> > {\n> > +\n>\n> Unrelated blank line\n>\n> > static const std::map\n> sensorProps = {\n> > { \"imx219\", {\n> > .unitCellSize = { 1120, 1120 },\n> > + .testPatternModeMap = {\n> > + { 0, controls::draft::TestPatternModeOff },\n> > + { 1,\n> controls::draft::TestPatternModeColorBars },\n> > + { 2,\n> controls::draft::TestPatternModeSolidColor },\n> > + { 3,\n> controls::draft::TestPatternModeColorBarsFadeToGray },\n> > + { 4, controls::draft::TestPatternModePn9 },\n> > + },\n>\n> This is from the imx219 driver\n>\n> #define IMX219_TEST_PATTERN_DISABLE 0\n> #define IMX219_TEST_PATTERN_SOLID_COLOR 1\n> #define IMX219_TEST_PATTERN_COLOR_BARS 2\n> #define IMX219_TEST_PATTERN_GREY_COLOR 3\n> #define IMX219_TEST_PATTERN_PN9 4\n>\n> Should you map 1 to controls::draft::TestPatternModeSolidColor and 2\n> to controls::draft::TestPatternModeColorBars ?\n>\n>\nThose values are for the imx219 register.\nInterestingly, imx_219_test_pattern_menu[] and imx_219_test_pattern_val[]\nare in different order from the values.\nI think my map is correct. This is very confusing. IMO, the different order\namong them doesn't make sense, which should be fixed.\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/imx219.c;l=395;drc=c90b4d70b1746f5a46fb7bad988731e604a44d4e\n\n\n> > } },\n> > { \"ov5670\", {\n> > .unitCellSize = { 1120, 1120 },\n> > + .testPatternModeMap = {\n> > + { 0, controls::draft::TestPatternModeOff },\n> > + { 1,\n> controls::draft::TestPatternModeColorBars },\n> > + },\n>\n> The driver only defines:\n> #define OV5670_TEST_PATTERN_ENABLE BIT(3)\n>\n> does this map to ColorBars ?\n>\n>\nThe value is a value to notify the ov5670 sensor tha a test pattern mode is\nenabled.\nThe array for th test pattern menu is\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/ov5670.c;l=1718;drc=eba08021e15076afc21b506e71e2f4e523f27f8c\n.\nI think my map is correct.\n\n\n> > +\n>\n> Unrelated blank line\n>\n> > } },\n> > { \"ov13858\", {\n> > .unitCellSize = { 1120, 1120 },\n> > + .testPatternModeMap = {\n> > + { 0, controls::draft::TestPatternModeOff },\n> > + { 1,\n> controls::draft::TestPatternModeColorBars },\n> > + { 2,\n> controls::draft::TestPatternModeColorBarsFadeToGray },\n>\n> The driver only reports 1 mode:\n> #define OV13858_TEST_PATTERN_ENABLE BIT(7)\n>\n> Where does the second come from ?\n>\n>\nSame. The value is a value to notify the ov13858 sensor tha a test pattern\nmode is enabled.\nThe array for the test pattern menu is\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/ov13858.c;l=929;drc=74c3ddd9887f60824891d2574a1689e8c13bf191\nI think my map is correct.\n\nThanks,\n-Hiro\n\n> Thanks\n> j\n>\n> > + },\n> > } },\n> > };\n> >\n> > --\n> > 2.31.1.607.g51e8a6a459-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 2360FC31FB\n\tfor ;\n\tWed, 19 May 2021 05:31:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F1ED68920;\n\tWed, 19 May 2021 07:31:37 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B9DE602B0\n\tfor ;\n\tWed, 19 May 2021 07:31:35 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id k14so14618364eji.2\n\tfor ;\n\tTue, 18 May 2021 22:31:35 -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=\"MBfR5//1\"; 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=8RqzIFdvxA+/AYNryBdzTbGQbbwtlnm+yqOnxWD+F1c=;\n\tb=MBfR5//19kU0f61BpwXUH2lYt+pN1tzB70ikScJPYK/C2RNStC3E+jJHGuy1VGBX9F\n\tUcq8Gi2LJXC9rrRJqLC07PepqbdLuAmxBWxKwUNwJznmPB5PK2W45mFPvrVllKH/cJ3R\n\tJj0Q9nSsYxkxBxDk3H0yiK6nSxjj/LfqARVds=","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=8RqzIFdvxA+/AYNryBdzTbGQbbwtlnm+yqOnxWD+F1c=;\n\tb=TS4+ENTEx5zmXakYNCkLYYse7LyaJedlplXFHOYIN2ShL3Y7v/npECeZ7CunVpWnZT\n\tHDXt9U6gbau++Q4b88fhVwfqDBa0W7fVxdUd2Ch+N2c65piMDwD2m977Uveqjflu04kR\n\tUHmGk9kGaURfXu30FC0UYljQc7XLlphi0tWywX+eazbC/9HuQr624K854iju1LSEFHZb\n\tifKRURDuwkivLLCHXuNpB6ZF5o1z980wkUWIAcc1fJfxEY5rC5Tzhu7CtBHrOiwcrjOh\n\t6Q9fMgMYfl3UWTJiFcx2tbfxpM+bMAQ7hn5ttBNqi0u/OzRu/1J3IqoZ+IemFmOVFceB\n\tVYUg==","X-Gm-Message-State":"AOAM533f9TN0ymK0vbGyuKMGp3SV0bnZ6L0W6uCEwlbRNepwiypva+ep\n\trUS76i8UcIWSZlqQRz4b0Jj9AKNGzMe8/TUkva/9AUXgfQ1OKw==","X-Google-Smtp-Source":"ABdhPJy1ThqnZAl+qBz9IzH+68kROqvMVG/xgSqbQHD5gIPcmcij8SKKGMOxZIFkOtIRuGzcNAlBmZG10kBSyETx/1E=","X-Received":"by 2002:a17:907:3ea0:: with SMTP id\n\ths32mr2585033ejc.475.1621402294780; \n\tTue, 18 May 2021 22:31:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210506075449.1761752-1-hiroh@chromium.org>\n\t<20210506075449.1761752-4-hiroh@chromium.org>\n\t<20210518122237.yyvtkuyx55t7x2se@uno.localdomain>","In-Reply-To":"<20210518122237.yyvtkuyx55t7x2se@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 19 May 2021 14:31:23 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000d19c9705c2a82351\"","Subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera:\n\tCameraSensorProperties: Adds table of v4l2 index and test pattern","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\" "}}]