[{"id":19389,"web_url":"https://patchwork.libcamera.org/comment/19389/","msgid":"","date":"2021-09-06T01:37:50","subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Martin,\n\nThank you for the patch.\n\nOn Thu, Sep 02, 2021 at 08:44:16AM +0200, Martin Kepplinger wrote:\n> Add camera sensor properties for the Hynix hi846 sensor.\n> The part is also called YACG4D0C9SHC and a datasheet can be found at\n> https://product.skhynix.com/products/cis/cis.go\n> \n> This is the selfie camera in the Librem 5 phone.\n> \n> Signed-off-by: Martin Kepplinger \n> ---\n> \n> Note that the driver is not yet merged into the mainline but being\n> reviewed:\n> https://lore.kernel.org/linux-media/20210831134344.1673318-1-martin.kepplinger@puri.sm/\n\nIt's well on its way to upstream so that's fine with me.\n\nReviewed-by: Laurent Pinchart \n\n> libcamera doesn't warn about anything when using the camera. `cam` streams\n> and qcam shows a (non-debayered) image.\n\nqcam supports debayering when using the GLES rendered (-r gles), but\ndoesn't yet support 16-bit formats. The following patch is completely\nuntested but may help. Would you be able to give it a try ?\n\ndiff --git a/src/qcam/assets/shader/bayer_8.frag b/src/qcam/assets/shader/bayer_8.frag\nindex 4ece44ab5650..b924e8c6ff0a 100644\n--- a/src/qcam/assets/shader/bayer_8.frag\n+++ b/src/qcam/assets/shader/bayer_8.frag\n@@ -22,10 +22,15 @@ varying vec4 center;\n varying vec4 yCoord;\n varying vec4 xCoord;\n \n+#if defined(BAYER_16BPP)\n+#define fetch(x, y) texture2D(tex_y, vec2(x, y)).a\n+#else\n+#define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n+#endif\n+\n void main(void) {\n- #define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n \n- float C = texture2D(tex_y, center.xy).r; // ( 0, 0)\n+ float C = fetch(center.x, center.y); // ( 0, 0)\n const vec4 kC = vec4( 4.0, 6.0, 5.0, 5.0) / 8.0;\n \n // Determine which of four types of pixels we are on.\ndiff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\nindex 32232faa2ad8..83ee0c7d54bf 100644\n--- a/src/qcam/viewfinder_gl.cpp\n+++ b/src/qcam/viewfinder_gl.cpp\n@@ -53,6 +53,11 @@ static const QList supportedFormats{\n \tlibcamera::formats::SGBRG12_CSI2P,\n \tlibcamera::formats::SGRBG12_CSI2P,\n \tlibcamera::formats::SRGGB12_CSI2P,\n+\t/* Raw Bayer 16-bit */\n+\tlibcamera::formats::SBGGR16,\n+\tlibcamera::formats::SGBRG16,\n+\tlibcamera::formats::SGRBG16,\n+\tlibcamera::formats::SRGGB16,\n };\n \n ViewFinderGL::ViewFinderGL(QWidget *parent)\n@@ -226,6 +231,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n \t\tfragmentShaderDefines_.append(\"#define RGB_PATTERN bgr\");\n \t\tfragmentShaderFile_ = \":RGB.frag\";\n \t\tbreak;\n+\tcase libcamera::formats::SBGGR16:\n+\t\tfragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n+\t\t[[fallthrough]];\n \tcase libcamera::formats::SBGGR8:\n \t\tfirstRed_.setX(1.0);\n \t\tfirstRed_.setY(1.0);\n@@ -233,6 +241,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n \t\tfragmentShaderFile_ = \":bayer_8.frag\";\n \t\ttextureMinMagFilters_ = GL_NEAREST;\n \t\tbreak;\n+\tcase libcamera::formats::SGBRG16:\n+\t\tfragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n+\t\t[[fallthrough]];\n \tcase libcamera::formats::SGBRG8:\n \t\tfirstRed_.setX(0.0);\n \t\tfirstRed_.setY(1.0);\n@@ -240,6 +251,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n \t\tfragmentShaderFile_ = \":bayer_8.frag\";\n \t\ttextureMinMagFilters_ = GL_NEAREST;\n \t\tbreak;\n+\tcase libcamera::formats::SGRBG16:\n+\t\tfragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n+\t\t[[fallthrough]];\n \tcase libcamera::formats::SGRBG8:\n \t\tfirstRed_.setX(1.0);\n \t\tfirstRed_.setY(0.0);\n@@ -247,6 +261,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n \t\tfragmentShaderFile_ = \":bayer_8.frag\";\n \t\ttextureMinMagFilters_ = GL_NEAREST;\n \t\tbreak;\n+\tcase libcamera::formats::SRGGB16:\n+\t\tfragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n+\t\t[[fallthrough]];\n \tcase libcamera::formats::SRGGB8:\n \t\tfirstRed_.setX(0.0);\n \t\tfirstRed_.setY(0.0);\n@@ -697,6 +714,36 @@ void ViewFinderGL::doRender()\n \t\t\t\t\t 1.0f / (size_.height() - 1));\n \t\tbreak;\n \n+\tcase libcamera::formats::SBGGR16:\n+\tcase libcamera::formats::SGBRG16:\n+\tcase libcamera::formats::SGRBG16:\n+\tcase libcamera::formats::SRGGB16:\n+\t\t/*\n+\t\t * Raw Bayer 16-bit formats are stored in a GL_LUMINANCE_ALPHA\n+\t\t * texture. The texture width is equal to half the stride.\n+\t\t */\n+\t\tglActiveTexture(GL_TEXTURE0);\n+\t\tconfigureTexture(*textures_[0]);\n+\t\tglTexImage2D(GL_TEXTURE_2D,\n+\t\t\t 0,\n+\t\t\t GL_LUMINANCE_ALPHA,\n+\t\t\t stride_ / 2,\n+\t\t\t size_.height(),\n+\t\t\t 0,\n+\t\t\t GL_LUMINANCE_ALPHA,\n+\t\t\t GL_UNSIGNED_BYTE,\n+\t\t\t image_->data(0).data());\n+\t\tshaderProgram_.setUniformValue(textureUniformY_, 0);\n+\t\tshaderProgram_.setUniformValue(textureUniformBayerFirstRed_,\n+\t\t\t\t\t firstRed_);\n+\t\tshaderProgram_.setUniformValue(textureUniformSize_,\n+\t\t\t\t\t size_.width(), /* in pixels */\n+\t\t\t\t\t size_.height());\n+\t\tshaderProgram_.setUniformValue(textureUniformStep_,\n+\t\t\t\t\t 1.0f / (stride_ / 2 - 1),\n+\t\t\t\t\t 1.0f / (size_.height() - 1));\n+\t\tbreak;\n+\n \tdefault:\n \t\tbreak;\n \t};\n\n\n> src/libcamera/camera_sensor_properties.cpp | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> index 4ee45e72..39bb282d 100644\n> --- a/src/libcamera/camera_sensor_properties.cpp\n> +++ b/src/libcamera/camera_sensor_properties.cpp\n> @@ -52,6 +52,24 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> const CameraSensorProperties *CameraSensorProperties::get(const std::string &sensor)\n> {\n> \tstatic const std::map sensorProps = {\n> +\t\t{ \"hi846\", {\n> +\t\t\t.unitCellSize = { 1120, 1120 },\n> +\t\t\t.testPatternModes = {\n> +\t\t\t\t{ 0, controls::draft::TestPatternModeOff },\n> +\t\t\t\t{ 1, controls::draft::TestPatternModeSolidColor },\n> +\t\t\t\t{ 2, controls::draft::TestPatternModeColorBars },\n> +\t\t\t\t{ 3, controls::draft::TestPatternModeColorBarsFadeToGray },\n> +\t\t\t\t{ 4, controls::draft::TestPatternModePn9 },\n> +\t\t\t\t/*\n> +\t\t\t\t * No corresponding test pattern mode for:\n> +\t\t\t\t * 5: \"Gradient Horizontal\"\n> +\t\t\t\t * 6: \"Gradient Vertical\"\n> +\t\t\t\t * 7: \"Check Board\"\n> +\t\t\t\t * 8: \"Slant Pattern\"\n> +\t\t\t\t * 9: \"Resolution Pattern\"\n> +\t\t\t\t */\n> +\t\t\t},\n> +\t\t} },\n> \t\t{ \"imx219\", {\n> \t\t\t.unitCellSize = { 1120, 1120 },\n> \t\t\t.testPatternModes = {","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 021AFBD87D\n\tfor ;\n\tMon, 6 Sep 2021 01:38:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65F4C69168;\n\tMon, 6 Sep 2021 03:38:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B17360254\n\tfor ;\n\tMon, 6 Sep 2021 03:38:08 +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 9678C317;\n\tMon, 6 Sep 2021 03:38:07 +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=\"LIzVzHb7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630892287;\n\tbh=Pqi8cY0VMlYuS1PkwEHyVxmjhXAZ9D3tzdyNs8d9jlM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LIzVzHb7iVvel7QiKQEbWXNyGXvE+9Ay+dnsNineF+tIwJLUOCzpcgad+UuZBb+yZ\n\tnTMaAIavYLmHi1ggq1Ti/h+LVK+7Leef0Bel5iVU6C8PkLD9GEFUB0B4uG1FZ5pGCF\n\t8Zg3fUvUpohQ6kI/7AvRSS9/U+BkX2fHXtJ71yGI=","Date":"Mon, 6 Sep 2021 04:37:50 +0300","From":"Laurent Pinchart ","To":"Martin Kepplinger ","Message-ID":"","References":"<20210902064416.6342-1-martin.kepplinger@puri.sm>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210902064416.6342-1-martin.kepplinger@puri.sm>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","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":19440,"web_url":"https://patchwork.libcamera.org/comment/19440/","msgid":"","date":"2021-09-06T12:51:47","subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","submitter":{"id":93,"url":"https://patchwork.libcamera.org/api/people/93/","name":"Martin Kepplinger","email":"martin.kepplinger@puri.sm"},"content":"Am Montag, dem 06.09.2021 um 04:37 +0300 schrieb Laurent Pinchart:\n> Hi Martin,\n> \n> Thank you for the patch.\n> \n> On Thu, Sep 02, 2021 at 08:44:16AM +0200, Martin Kepplinger wrote:\n> > Add camera sensor properties for the Hynix hi846 sensor.\n> > The part is also called YACG4D0C9SHC and a datasheet can be found\n> > at\n> > https://product.skhynix.com/products/cis/cis.go\n> > \n> > This is the selfie camera in the Librem 5 phone.\n> > \n> > Signed-off-by: Martin Kepplinger \n> > ---\n> > \n> > Note that the driver is not yet merged into the mainline but being\n> > reviewed:\n> > https://lore.kernel.org/linux-media/20210831134344.1673318-1-martin.kepplinger@puri.sm/\n> \n> It's well on its way to upstream so that's fine with me.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > libcamera doesn't warn about anything when using the camera. `cam`\n> > streams\n> > and qcam shows a (non-debayered) image.\n> \n> qcam supports debayering when using the GLES rendered (-r gles), but\n> doesn't yet support 16-bit formats. The following patch is completely\n> untested but may help. Would you be able to give it a try ?\n> \n> diff --git a/src/qcam/assets/shader/bayer_8.frag\n> b/src/qcam/assets/shader/bayer_8.frag\n> index 4ece44ab5650..b924e8c6ff0a 100644\n> --- a/src/qcam/assets/shader/bayer_8.frag\n> +++ b/src/qcam/assets/shader/bayer_8.frag\n> @@ -22,10 +22,15 @@ varying vec4            center;\n>  varying vec4            yCoord;\n>  varying vec4            xCoord;\n>  \n> +#if defined(BAYER_16BPP)\n> +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).a\n> +#else\n> +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n> +#endif\n> +\n>  void main(void) {\n> -    #define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n>  \n> -    float C = texture2D(tex_y, center.xy).r; // ( 0, 0)\n> +    float C = fetch(center.x, center.y); // ( 0, 0)\n>      const vec4 kC = vec4( 4.0,  6.0,  5.0,  5.0) / 8.0;\n>  \n>      // Determine which of four types of pixels we are on.\n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index 32232faa2ad8..83ee0c7d54bf 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -53,6 +53,11 @@ static const QList\n> supportedFormats{\n>         libcamera::formats::SGBRG12_CSI2P,\n>         libcamera::formats::SGRBG12_CSI2P,\n>         libcamera::formats::SRGGB12_CSI2P,\n> +       /* Raw Bayer 16-bit */\n> +       libcamera::formats::SBGGR16,\n> +       libcamera::formats::SGBRG16,\n> +       libcamera::formats::SGRBG16,\n> +       libcamera::formats::SRGGB16,\n>  };\n>  \n>  ViewFinderGL::ViewFinderGL(QWidget *parent)\n> @@ -226,6 +231,9 @@ bool ViewFinderGL::selectFormat(const\n> libcamera::PixelFormat &format)\n>                 fragmentShaderDefines_.append(\"#define RGB_PATTERN\n> bgr\");\n>                 fragmentShaderFile_ = \":RGB.frag\";\n>                 break;\n> +       case libcamera::formats::SBGGR16:\n> +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> +               [[fallthrough]];\n>         case libcamera::formats::SBGGR8:\n>                 firstRed_.setX(1.0);\n>                 firstRed_.setY(1.0);\n> @@ -233,6 +241,9 @@ bool ViewFinderGL::selectFormat(const\n> libcamera::PixelFormat &format)\n>                 fragmentShaderFile_ = \":bayer_8.frag\";\n>                 textureMinMagFilters_ = GL_NEAREST;\n>                 break;\n> +       case libcamera::formats::SGBRG16:\n> +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> +               [[fallthrough]];\n>         case libcamera::formats::SGBRG8:\n>                 firstRed_.setX(0.0);\n>                 firstRed_.setY(1.0);\n> @@ -240,6 +251,9 @@ bool ViewFinderGL::selectFormat(const\n> libcamera::PixelFormat &format)\n>                 fragmentShaderFile_ = \":bayer_8.frag\";\n>                 textureMinMagFilters_ = GL_NEAREST;\n>                 break;\n> +       case libcamera::formats::SGRBG16:\n> +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> +               [[fallthrough]];\n>         case libcamera::formats::SGRBG8:\n>                 firstRed_.setX(1.0);\n>                 firstRed_.setY(0.0);\n> @@ -247,6 +261,9 @@ bool ViewFinderGL::selectFormat(const\n> libcamera::PixelFormat &format)\n>                 fragmentShaderFile_ = \":bayer_8.frag\";\n>                 textureMinMagFilters_ = GL_NEAREST;\n>                 break;\n> +       case libcamera::formats::SRGGB16:\n> +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> +               [[fallthrough]];\n>         case libcamera::formats::SRGGB8:\n>                 firstRed_.setX(0.0);\n>                 firstRed_.setY(0.0);\n> @@ -697,6 +714,36 @@ void ViewFinderGL::doRender()\n>                                                1.0f / (size_.height()\n> - 1));\n>                 break;\n>  \n> +       case libcamera::formats::SBGGR16:\n> +       case libcamera::formats::SGBRG16:\n> +       case libcamera::formats::SGRBG16:\n> +       case libcamera::formats::SRGGB16:\n> +               /*\n> +                * Raw Bayer 16-bit formats are stored in a\n> GL_LUMINANCE_ALPHA\n> +                * texture. The texture width is equal to half the\n> stride.\n> +                */\n> +               glActiveTexture(GL_TEXTURE0);\n> +               configureTexture(*textures_[0]);\n> +               glTexImage2D(GL_TEXTURE_2D,\n> +                            0,\n> +                            GL_LUMINANCE_ALPHA,\n> +                            stride_ / 2,\n> +                            size_.height(),\n> +                            0,\n> +                            GL_LUMINANCE_ALPHA,\n> +                            GL_UNSIGNED_BYTE,\n> +                            image_->data(0).data());\n\nbuilds when using data_ here (if that's correct?)\n\nwithout further inspection, qcam shows a black image stream with this\n(the ugly debayered image at least shows something so gain should be\nenough to see something).\n\nthe fps are quite good though :)\n\n\n\n> +               shaderProgram_.setUniformValue(textureUniformY_, 0);\n> +               shaderProgram_.setUniformValue(textureUniformBayerFir\n> stRed_,\n> +                                              firstRed_);\n> +               shaderProgram_.setUniformValue(textureUniformSize_,\n> +                                              size_.width(), /* in\n> pixels */\n> +                                              size_.height());\n> +               shaderProgram_.setUniformValue(textureUniformStep_,\n> +                                              1.0f / (stride_ / 2 -\n> 1),\n> +                                              1.0f / (size_.height()\n> - 1));\n> +               break;\n> +\n>         default:\n>                 break;\n>         };\n> \n> \n> >  src/libcamera/camera_sensor_properties.cpp | 18 ++++++++++++++++++\n> >  1 file changed, 18 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> > b/src/libcamera/camera_sensor_properties.cpp\n> > index 4ee45e72..39bb282d 100644\n> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > @@ -52,6 +52,24 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> >  const CameraSensorProperties *CameraSensorProperties::get(const\n> > std::string &sensor)\n> >  {\n> >         static const std::map > CameraSensorProperties> sensorProps = {\n> > +               { \"hi846\", {\n> > +                       .unitCellSize = { 1120, 1120 },\n> > +                       .testPatternModes = {\n> > +                               { 0,\n> > controls::draft::TestPatternModeOff },\n> > +                               { 1,\n> > controls::draft::TestPatternModeSolidColor },\n> > +                               { 2,\n> > controls::draft::TestPatternModeColorBars },\n> > +                               { 3,\n> > controls::draft::TestPatternModeColorBarsFadeToGray },\n> > +                               { 4,\n> > controls::draft::TestPatternModePn9 },\n> > +                               /*\n> > +                                * No corresponding test pattern\n> > mode for:\n> > +                                * 5: \"Gradient Horizontal\"\n> > +                                * 6: \"Gradient Vertical\"\n> > +                                * 7: \"Check Board\"\n> > +                                * 8: \"Slant Pattern\"\n> > +                                * 9: \"Resolution Pattern\"\n> > +                                */\n> > +                       },\n> > +               } },\n> >                 { \"imx219\", {\n> >                         .unitCellSize = { 1120, 1120 },\n> >                         .testPatternModes = {\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 4E1E3BD87D\n\tfor ;\n\tMon, 6 Sep 2021 12:52:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 719466916A;\n\tMon, 6 Sep 2021 14:52:00 +0200 (CEST)","from comms.puri.sm (comms.puri.sm [159.203.221.185])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 123B360137\n\tfor ;\n\tMon, 6 Sep 2021 14:51:58 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n\tby comms.puri.sm (Postfix) with ESMTP id F1D47DFB4C;\n\tMon, 6 Sep 2021 05:51:56 -0700 (PDT)","from comms.puri.sm ([127.0.0.1])\n\tby localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id Y1qquugi3gnS; Mon, 6 Sep 2021 05:51:52 -0700 (PDT)"],"Message-ID":"","From":"Martin Kepplinger ","To":"Laurent Pinchart ","Date":"Mon, 06 Sep 2021 14:51:47 +0200","In-Reply-To":"","References":"<20210902064416.6342-1-martin.kepplinger@puri.sm>\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.38.3-1 ","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","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":19479,"web_url":"https://patchwork.libcamera.org/comment/19479/","msgid":"","date":"2021-09-06T18:04:47","subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Martin,\n\nOn Mon, Sep 06, 2021 at 02:51:47PM +0200, Martin Kepplinger wrote:\n> Am Montag, dem 06.09.2021 um 04:37 +0300 schrieb Laurent Pinchart:\n> > On Thu, Sep 02, 2021 at 08:44:16AM +0200, Martin Kepplinger wrote:\n> > > Add camera sensor properties for the Hynix hi846 sensor.\n> > > The part is also called YACG4D0C9SHC and a datasheet can be found\n> > > at\n> > > https://product.skhynix.com/products/cis/cis.go\n> > > \n> > > This is the selfie camera in the Librem 5 phone.\n> > > \n> > > Signed-off-by: Martin Kepplinger \n> > > ---\n> > > \n> > > Note that the driver is not yet merged into the mainline but being\n> > > reviewed:\n> > > https://lore.kernel.org/linux-media/20210831134344.1673318-1-martin.kepplinger@puri.sm/\n> > \n> > It's well on its way to upstream so that's fine with me.\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > > libcamera doesn't warn about anything when using the camera. `cam`\n> > > streams\n> > > and qcam shows a (non-debayered) image.\n> > \n> > qcam supports debayering when using the GLES rendered (-r gles), but\n> > doesn't yet support 16-bit formats. The following patch is completely\n> > untested but may help. Would you be able to give it a try ?\n> > \n> > diff --git a/src/qcam/assets/shader/bayer_8.frag b/src/qcam/assets/shader/bayer_8.frag\n> > index 4ece44ab5650..b924e8c6ff0a 100644\n> > --- a/src/qcam/assets/shader/bayer_8.frag\n> > +++ b/src/qcam/assets/shader/bayer_8.frag\n> > @@ -22,10 +22,15 @@ varying vec4            center;\n> >  varying vec4            yCoord;\n> >  varying vec4            xCoord;\n> >  \n> > +#if defined(BAYER_16BPP)\n> > +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).a\n> > +#else\n> > +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n> > +#endif\n> > +\n> >  void main(void) {\n> > -    #define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n> >  \n> > -    float C = texture2D(tex_y, center.xy).r; // ( 0, 0)\n> > +    float C = fetch(center.x, center.y); // ( 0, 0)\n> >      const vec4 kC = vec4( 4.0,  6.0,  5.0,  5.0) / 8.0;\n> >  \n> >      // Determine which of four types of pixels we are on.\n> > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > index 32232faa2ad8..83ee0c7d54bf 100644\n> > --- a/src/qcam/viewfinder_gl.cpp\n> > +++ b/src/qcam/viewfinder_gl.cpp\n> > @@ -53,6 +53,11 @@ static const QList supportedFormats{\n> >         libcamera::formats::SGBRG12_CSI2P,\n> >         libcamera::formats::SGRBG12_CSI2P,\n> >         libcamera::formats::SRGGB12_CSI2P,\n> > +       /* Raw Bayer 16-bit */\n> > +       libcamera::formats::SBGGR16,\n> > +       libcamera::formats::SGBRG16,\n> > +       libcamera::formats::SGRBG16,\n> > +       libcamera::formats::SRGGB16,\n> >  };\n> >  \n> >  ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > @@ -226,6 +231,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> >                 fragmentShaderDefines_.append(\"#define RGB_PATTERN bgr\");\n> >                 fragmentShaderFile_ = \":RGB.frag\";\n> >                 break;\n> > +       case libcamera::formats::SBGGR16:\n> > +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> > +               [[fallthrough]];\n> >         case libcamera::formats::SBGGR8:\n> >                 firstRed_.setX(1.0);\n> >                 firstRed_.setY(1.0);\n> > @@ -233,6 +241,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> >                 textureMinMagFilters_ = GL_NEAREST;\n> >                 break;\n> > +       case libcamera::formats::SGBRG16:\n> > +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> > +               [[fallthrough]];\n> >         case libcamera::formats::SGBRG8:\n> >                 firstRed_.setX(0.0);\n> >                 firstRed_.setY(1.0);\n> > @@ -240,6 +251,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> >                 textureMinMagFilters_ = GL_NEAREST;\n> >                 break;\n> > +       case libcamera::formats::SGRBG16:\n> > +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> > +               [[fallthrough]];\n> >         case libcamera::formats::SGRBG8:\n> >                 firstRed_.setX(1.0);\n> >                 firstRed_.setY(0.0);\n> > @@ -247,6 +261,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> >                 textureMinMagFilters_ = GL_NEAREST;\n> >                 break;\n> > +       case libcamera::formats::SRGGB16:\n> > +               fragmentShaderDefines_.append(\"#define BAYER_16BPP\");\n> > +               [[fallthrough]];\n> >         case libcamera::formats::SRGGB8:\n> >                 firstRed_.setX(0.0);\n> >                 firstRed_.setY(0.0);\n> > @@ -697,6 +714,36 @@ void ViewFinderGL::doRender()\n> >                                                1.0f / (size_.height() - 1));\n> >                 break;\n> >  \n> > +       case libcamera::formats::SBGGR16:\n> > +       case libcamera::formats::SGBRG16:\n> > +       case libcamera::formats::SGRBG16:\n> > +       case libcamera::formats::SRGGB16:\n> > +               /*\n> > +                * Raw Bayer 16-bit formats are stored in a GL_LUMINANCE_ALPHA\n> > +                * texture. The texture width is equal to half the stride.\n> > +                */\n> > +               glActiveTexture(GL_TEXTURE0);\n> > +               configureTexture(*textures_[0]);\n> > +               glTexImage2D(GL_TEXTURE_2D,\n> > +                            0,\n> > +                            GL_LUMINANCE_ALPHA,\n> > +                            stride_ / 2,\n> > +                            size_.height(),\n> > +                            0,\n> > +                            GL_LUMINANCE_ALPHA,\n> > +                            GL_UNSIGNED_BYTE,\n> > +                            image_->data(0).data());\n> \n> builds when using data_ here (if that's correct?)\n\nIt is. The diff was based on a branch that hasn't been merged yet, I\nforgot about it. Sorry.\n\n> without further inspection, qcam shows a black image stream with this\n> (the ugly debayered image at least shows something so gain should be\n> enough to see something).\n\nDo I recall correctly that the imx7 CSI bridge driver advertises 16-bit\nBayer formats, but aligns the data to the right, not to the left ? That\nshould then be fixed in the driver, and this patch will require more\nintrusive changes in the shader.\n\nAs a quick test, could you drop the change to\nsrc/qcam/assets/shader/bayer_8.frag and try again ? I expect artifacts,\nbut a visible image.\n\n> the fps are quite good though :)\n> \n> > +               shaderProgram_.setUniformValue(textureUniformY_, 0);\n> > +               shaderProgram_.setUniformValue(textureUniformBayerFirstRed_,\n> > +                                              firstRed_);\n> > +               shaderProgram_.setUniformValue(textureUniformSize_,\n> > +                                              size_.width(), /* in pixels */\n> > +                                              size_.height());\n> > +               shaderProgram_.setUniformValue(textureUniformStep_,\n> > +                                              1.0f / (stride_ / 2 - 1),\n> > +                                              1.0f / (size_.height() - 1));\n> > +               break;\n> > +\n> >         default:\n> >                 break;\n> >         };\n> > \n> > >  src/libcamera/camera_sensor_properties.cpp | 18 ++++++++++++++++++\n> > >  1 file changed, 18 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > > index 4ee45e72..39bb282d 100644\n> > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > @@ -52,6 +52,24 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> > >  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sensor)\n> > >  {\n> > >         static const std::map sensorProps = {\n> > > +               { \"hi846\", {\n> > > +                       .unitCellSize = { 1120, 1120 },\n> > > +                       .testPatternModes = {\n> > > +                               { 0, controls::draft::TestPatternModeOff },\n> > > +                               { 1, controls::draft::TestPatternModeSolidColor },\n> > > +                               { 2, controls::draft::TestPatternModeColorBars },\n> > > +                               { 3, controls::draft::TestPatternModeColorBarsFadeToGray },\n> > > +                               { 4, controls::draft::TestPatternModePn9 },\n> > > +                               /*\n> > > +                                * No corresponding test pattern mode for:\n> > > +                                * 5: \"Gradient Horizontal\"\n> > > +                                * 6: \"Gradient Vertical\"\n> > > +                                * 7: \"Check Board\"\n> > > +                                * 8: \"Slant Pattern\"\n> > > +                                * 9: \"Resolution Pattern\"\n> > > +                                */\n> > > +                       },\n> > > +               } },\n> > >                 { \"imx219\", {\n> > >                         .unitCellSize = { 1120, 1120 },\n> > >                         .testPatternModes = {","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 D2D7DBD87D\n\tfor ;\n\tMon, 6 Sep 2021 18:05:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F8B56916C;\n\tMon, 6 Sep 2021 20:05:08 +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 E464A60137\n\tfor ;\n\tMon, 6 Sep 2021 20:05:06 +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 4F45E8AD;\n\tMon, 6 Sep 2021 20:05:06 +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=\"kXDrZfsH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630951506;\n\tbh=6J0ASAkb0Coveyyy2yL+ClBlx0h1c1v4QXmoOk6UFTE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kXDrZfsHdJx4izzT1lTvSqx5Roiba0B6DAqYBpow5azbEXLQBSxq18Q5MPcbwBHTr\n\tABcM6/iMCQkCRTcb6tuaiAtrUJUjO5w1tcP8hQO0n7I9vWtIjQANw2/xr7JXKgaCRq\n\tCdqXl7e3O1aLEumRz2p1MCaZMtEFFv9Gzo53uuqU=","Date":"Mon, 6 Sep 2021 21:04:47 +0300","From":"Laurent Pinchart ","To":"Martin Kepplinger ","Message-ID":"","References":"<20210902064416.6342-1-martin.kepplinger@puri.sm>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","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":19510,"web_url":"https://patchwork.libcamera.org/comment/19510/","msgid":"<5b548ca515ae94458edd17b47c08640938a04b4f.camel@puri.sm>","date":"2021-09-07T13:16:13","subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","submitter":{"id":93,"url":"https://patchwork.libcamera.org/api/people/93/","name":"Martin Kepplinger","email":"martin.kepplinger@puri.sm"},"content":"Am Montag, dem 06.09.2021 um 21:04 +0300 schrieb Laurent Pinchart:\n> Hi Martin,\n> \n> On Mon, Sep 06, 2021 at 02:51:47PM +0200, Martin Kepplinger wrote:\n> > Am Montag, dem 06.09.2021 um 04:37 +0300 schrieb Laurent Pinchart:\n> > > On Thu, Sep 02, 2021 at 08:44:16AM +0200, Martin Kepplinger\n> > > wrote:\n> > > > Add camera sensor properties for the Hynix hi846 sensor.\n> > > > The part is also called YACG4D0C9SHC and a datasheet can be\n> > > > found\n> > > > at\n> > > > https://product.skhynix.com/products/cis/cis.go\n> > > > \n> > > > This is the selfie camera in the Librem 5 phone.\n> > > > \n> > > > Signed-off-by: Martin Kepplinger \n> > > > ---\n> > > > \n> > > > Note that the driver is not yet merged into the mainline but\n> > > > being\n> > > > reviewed:\n> > > > https://lore.kernel.org/linux-media/20210831134344.1673318-1-martin.kepplinger@puri.sm/\n> > > \n> > > It's well on its way to upstream so that's fine with me.\n> > > \n> > > Reviewed-by: Laurent Pinchart \n> > > \n> > > > libcamera doesn't warn about anything when using the camera.\n> > > > `cam`\n> > > > streams\n> > > > and qcam shows a (non-debayered) image.\n> > > \n> > > qcam supports debayering when using the GLES rendered (-r gles),\n> > > but\n> > > doesn't yet support 16-bit formats. The following patch is\n> > > completely\n> > > untested but may help. Would you be able to give it a try ?\n> > > \n> > > diff --git a/src/qcam/assets/shader/bayer_8.frag\n> > > b/src/qcam/assets/shader/bayer_8.frag\n> > > index 4ece44ab5650..b924e8c6ff0a 100644\n> > > --- a/src/qcam/assets/shader/bayer_8.frag\n> > > +++ b/src/qcam/assets/shader/bayer_8.frag\n> > > @@ -22,10 +22,15 @@ varying vec4            center;\n> > >  varying vec4            yCoord;\n> > >  varying vec4            xCoord;\n> > >  \n> > > +#if defined(BAYER_16BPP)\n> > > +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).a\n> > > +#else\n> > > +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n> > > +#endif\n> > > +\n> > >  void main(void) {\n> > > -    #define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n> > >  \n> > > -    float C = texture2D(tex_y, center.xy).r; // ( 0, 0)\n> > > +    float C = fetch(center.x, center.y); // ( 0, 0)\n> > >      const vec4 kC = vec4( 4.0,  6.0,  5.0,  5.0) / 8.0;\n> > >  \n> > >      // Determine which of four types of pixels we are on.\n> > > diff --git a/src/qcam/viewfinder_gl.cpp\n> > > b/src/qcam/viewfinder_gl.cpp\n> > > index 32232faa2ad8..83ee0c7d54bf 100644\n> > > --- a/src/qcam/viewfinder_gl.cpp\n> > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > @@ -53,6 +53,11 @@ static const QList\n> > > supportedFormats{\n> > >         libcamera::formats::SGBRG12_CSI2P,\n> > >         libcamera::formats::SGRBG12_CSI2P,\n> > >         libcamera::formats::SRGGB12_CSI2P,\n> > > +       /* Raw Bayer 16-bit */\n> > > +       libcamera::formats::SBGGR16,\n> > > +       libcamera::formats::SGBRG16,\n> > > +       libcamera::formats::SGRBG16,\n> > > +       libcamera::formats::SRGGB16,\n> > >  };\n> > >  \n> > >  ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > > @@ -226,6 +231,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderDefines_.append(\"#define\n> > > RGB_PATTERN bgr\");\n> > >                 fragmentShaderFile_ = \":RGB.frag\";\n> > >                 break;\n> > > +       case libcamera::formats::SBGGR16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SBGGR8:\n> > >                 firstRed_.setX(1.0);\n> > >                 firstRed_.setY(1.0);\n> > > @@ -233,6 +241,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> > >                 textureMinMagFilters_ = GL_NEAREST;\n> > >                 break;\n> > > +       case libcamera::formats::SGBRG16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SGBRG8:\n> > >                 firstRed_.setX(0.0);\n> > >                 firstRed_.setY(1.0);\n> > > @@ -240,6 +251,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> > >                 textureMinMagFilters_ = GL_NEAREST;\n> > >                 break;\n> > > +       case libcamera::formats::SGRBG16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SGRBG8:\n> > >                 firstRed_.setX(1.0);\n> > >                 firstRed_.setY(0.0);\n> > > @@ -247,6 +261,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> > >                 textureMinMagFilters_ = GL_NEAREST;\n> > >                 break;\n> > > +       case libcamera::formats::SRGGB16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SRGGB8:\n> > >                 firstRed_.setX(0.0);\n> > >                 firstRed_.setY(0.0);\n> > > @@ -697,6 +714,36 @@ void ViewFinderGL::doRender()\n> > >                                                1.0f /\n> > > (size_.height() - 1));\n> > >                 break;\n> > >  \n> > > +       case libcamera::formats::SBGGR16:\n> > > +       case libcamera::formats::SGBRG16:\n> > > +       case libcamera::formats::SGRBG16:\n> > > +       case libcamera::formats::SRGGB16:\n> > > +               /*\n> > > +                * Raw Bayer 16-bit formats are stored in a\n> > > GL_LUMINANCE_ALPHA\n> > > +                * texture. The texture width is equal to half\n> > > the stride.\n> > > +                */\n> > > +               glActiveTexture(GL_TEXTURE0);\n> > > +               configureTexture(*textures_[0]);\n> > > +               glTexImage2D(GL_TEXTURE_2D,\n> > > +                            0,\n> > > +                            GL_LUMINANCE_ALPHA,\n> > > +                            stride_ / 2,\n> > > +                            size_.height(),\n> > > +                            0,\n> > > +                            GL_LUMINANCE_ALPHA,\n> > > +                            GL_UNSIGNED_BYTE,\n> > > +                            image_->data(0).data());\n> > \n> > builds when using data_ here (if that's correct?)\n> \n> It is. The diff was based on a branch that hasn't been merged yet, I\n> forgot about it. Sorry.\n> \n> > without further inspection, qcam shows a black image stream with\n> > this\n> > (the ugly debayered image at least shows something so gain should\n> > be\n> > enough to see something).\n> \n> Do I recall correctly that the imx7 CSI bridge driver advertises 16-\n> bit\n> Bayer formats, but aligns the data to the right, not to the left ?\n> That\n> should then be fixed in the driver, and this patch will require more\n> intrusive changes in the shader.\n\nexactly, the 16 bits start with zeroes for a 10-bit bayer format.\n\n> \n> As a quick test, could you drop the change to\n> src/qcam/assets/shader/bayer_8.frag and try again ? I expect\n> artifacts,\n> but a visible image.\n> \n> \n\nYou're right. a visible image with artifacts now.\n\nthanks,\n martin","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 0797FBE175\n\tfor ;\n\tTue, 7 Sep 2021 13:16:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FC466916A;\n\tTue, 7 Sep 2021 15:16:51 +0200 (CEST)","from comms.puri.sm (comms.puri.sm [159.203.221.185])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4CEDA60251\n\tfor ;\n\tTue, 7 Sep 2021 15:16:50 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n\tby comms.puri.sm (Postfix) with ESMTP id 48250DF77D;\n\tTue, 7 Sep 2021 06:16:18 -0700 (PDT)","from comms.puri.sm ([127.0.0.1])\n\tby localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id zDbdyU50wQZ4; Tue, 7 Sep 2021 06:16:17 -0700 (PDT)"],"Message-ID":"<5b548ca515ae94458edd17b47c08640938a04b4f.camel@puri.sm>","From":"Martin Kepplinger ","To":"Laurent Pinchart ","Date":"Tue, 07 Sep 2021 15:16:13 +0200","In-Reply-To":"","References":"<20210902064416.6342-1-martin.kepplinger@puri.sm>\n\t\n\t\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.38.3-1 ","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","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":19565,"web_url":"https://patchwork.libcamera.org/comment/19565/","msgid":"<3f5e16ced7806af5cbf2b211573ced887080cb36.camel@puri.sm>","date":"2021-09-09T07:04:09","subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","submitter":{"id":93,"url":"https://patchwork.libcamera.org/api/people/93/","name":"Martin Kepplinger","email":"martin.kepplinger@puri.sm"},"content":"Am Montag, dem 06.09.2021 um 21:04 +0300 schrieb Laurent Pinchart:\n> Hi Martin,\n> \n> On Mon, Sep 06, 2021 at 02:51:47PM +0200, Martin Kepplinger wrote:\n> > Am Montag, dem 06.09.2021 um 04:37 +0300 schrieb Laurent Pinchart:\n> > > On Thu, Sep 02, 2021 at 08:44:16AM +0200, Martin Kepplinger\n> > > wrote:\n> > > > Add camera sensor properties for the Hynix hi846 sensor.\n> > > > The part is also called YACG4D0C9SHC and a datasheet can be\n> > > > found\n> > > > at\n> > > > https://product.skhynix.com/products/cis/cis.go\n> > > > \n> > > > This is the selfie camera in the Librem 5 phone.\n> > > > \n> > > > Signed-off-by: Martin Kepplinger \n> > > > ---\n> > > > \n> > > > Note that the driver is not yet merged into the mainline but\n> > > > being\n> > > > reviewed:\n> > > > https://lore.kernel.org/linux-media/20210831134344.1673318-1-martin.kepplinger@puri.sm/\n> > > \n> > > It's well on its way to upstream so that's fine with me.\n> > > \n> > > Reviewed-by: Laurent Pinchart \n> > > \n> > > > libcamera doesn't warn about anything when using the camera.\n> > > > `cam`\n> > > > streams\n> > > > and qcam shows a (non-debayered) image.\n> > > \n> > > qcam supports debayering when using the GLES rendered (-r gles),\n> > > but\n> > > doesn't yet support 16-bit formats. The following patch is\n> > > completely\n> > > untested but may help. Would you be able to give it a try ?\n> > > \n> > > diff --git a/src/qcam/assets/shader/bayer_8.frag\n> > > b/src/qcam/assets/shader/bayer_8.frag\n> > > index 4ece44ab5650..b924e8c6ff0a 100644\n> > > --- a/src/qcam/assets/shader/bayer_8.frag\n> > > +++ b/src/qcam/assets/shader/bayer_8.frag\n> > > @@ -22,10 +22,15 @@ varying vec4            center;\n> > >  varying vec4            yCoord;\n> > >  varying vec4            xCoord;\n> > >  \n> > > +#if defined(BAYER_16BPP)\n> > > +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).a\n> > > +#else\n> > > +#define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n> > > +#endif\n> > > +\n> > >  void main(void) {\n> > > -    #define fetch(x, y) texture2D(tex_y, vec2(x, y)).r\n> > >  \n> > > -    float C = texture2D(tex_y, center.xy).r; // ( 0, 0)\n> > > +    float C = fetch(center.x, center.y); // ( 0, 0)\n> > >      const vec4 kC = vec4( 4.0,  6.0,  5.0,  5.0) / 8.0;\n> > >  \n> > >      // Determine which of four types of pixels we are on.\n> > > diff --git a/src/qcam/viewfinder_gl.cpp\n> > > b/src/qcam/viewfinder_gl.cpp\n> > > index 32232faa2ad8..83ee0c7d54bf 100644\n> > > --- a/src/qcam/viewfinder_gl.cpp\n> > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > @@ -53,6 +53,11 @@ static const QList\n> > > supportedFormats{\n> > >         libcamera::formats::SGBRG12_CSI2P,\n> > >         libcamera::formats::SGRBG12_CSI2P,\n> > >         libcamera::formats::SRGGB12_CSI2P,\n> > > +       /* Raw Bayer 16-bit */\n> > > +       libcamera::formats::SBGGR16,\n> > > +       libcamera::formats::SGBRG16,\n> > > +       libcamera::formats::SGRBG16,\n> > > +       libcamera::formats::SRGGB16,\n> > >  };\n> > >  \n> > >  ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > > @@ -226,6 +231,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderDefines_.append(\"#define\n> > > RGB_PATTERN bgr\");\n> > >                 fragmentShaderFile_ = \":RGB.frag\";\n> > >                 break;\n> > > +       case libcamera::formats::SBGGR16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SBGGR8:\n> > >                 firstRed_.setX(1.0);\n> > >                 firstRed_.setY(1.0);\n> > > @@ -233,6 +241,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> > >                 textureMinMagFilters_ = GL_NEAREST;\n> > >                 break;\n> > > +       case libcamera::formats::SGBRG16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SGBRG8:\n> > >                 firstRed_.setX(0.0);\n> > >                 firstRed_.setY(1.0);\n> > > @@ -240,6 +251,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> > >                 textureMinMagFilters_ = GL_NEAREST;\n> > >                 break;\n> > > +       case libcamera::formats::SGRBG16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SGRBG8:\n> > >                 firstRed_.setX(1.0);\n> > >                 firstRed_.setY(0.0);\n> > > @@ -247,6 +261,9 @@ bool ViewFinderGL::selectFormat(const\n> > > libcamera::PixelFormat &format)\n> > >                 fragmentShaderFile_ = \":bayer_8.frag\";\n> > >                 textureMinMagFilters_ = GL_NEAREST;\n> > >                 break;\n> > > +       case libcamera::formats::SRGGB16:\n> > > +               fragmentShaderDefines_.append(\"#define\n> > > BAYER_16BPP\");\n> > > +               [[fallthrough]];\n> > >         case libcamera::formats::SRGGB8:\n> > >                 firstRed_.setX(0.0);\n> > >                 firstRed_.setY(0.0);\n> > > @@ -697,6 +714,36 @@ void ViewFinderGL::doRender()\n> > >                                                1.0f /\n> > > (size_.height() - 1));\n> > >                 break;\n> > >  \n> > > +       case libcamera::formats::SBGGR16:\n> > > +       case libcamera::formats::SGBRG16:\n> > > +       case libcamera::formats::SGRBG16:\n> > > +       case libcamera::formats::SRGGB16:\n> > > +               /*\n> > > +                * Raw Bayer 16-bit formats are stored in a\n> > > GL_LUMINANCE_ALPHA\n> > > +                * texture. The texture width is equal to half\n> > > the stride.\n> > > +                */\n> > > +               glActiveTexture(GL_TEXTURE0);\n> > > +               configureTexture(*textures_[0]);\n> > > +               glTexImage2D(GL_TEXTURE_2D,\n> > > +                            0,\n> > > +                            GL_LUMINANCE_ALPHA,\n> > > +                            stride_ / 2,\n> > > +                            size_.height(),\n> > > +                            0,\n> > > +                            GL_LUMINANCE_ALPHA,\n> > > +                            GL_UNSIGNED_BYTE,\n> > > +                            image_->data(0).data());\n> > \n> > builds when using data_ here (if that's correct?)\n> \n> It is. The diff was based on a branch that hasn't been merged yet, I\n> forgot about it. Sorry.\n> \n> > without further inspection, qcam shows a black image stream with\n> > this\n> > (the ugly debayered image at least shows something so gain should\n> > be\n> > enough to see something).\n> \n> Do I recall correctly that the imx7 CSI bridge driver advertises 16-\n> bit\n> Bayer formats, but aligns the data to the right, not to the left ?\n> That\n> should then be fixed in the driver, and this patch will require more\n> intrusive changes in the shader.\n> \n\nWhat exactly would you want to fix in the driver here? The csi bridge\ndriver?\n\nthanks,\n\n martin","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 1FDD9BDB1D\n\tfor ;\n\tThu, 9 Sep 2021 07:04:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 978D969170;\n\tThu, 9 Sep 2021 09:04:16 +0200 (CEST)","from comms.puri.sm (comms.puri.sm [159.203.221.185])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8FA0F6024E\n\tfor ;\n\tThu, 9 Sep 2021 09:04:14 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n\tby comms.puri.sm (Postfix) with ESMTP id AEA47E1147;\n\tThu, 9 Sep 2021 00:04:12 -0700 (PDT)","from comms.puri.sm ([127.0.0.1])\n\tby localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id xvbEUc45K-Qr; Thu, 9 Sep 2021 00:04:11 -0700 (PDT)"],"Message-ID":"<3f5e16ced7806af5cbf2b211573ced887080cb36.camel@puri.sm>","From":"Martin Kepplinger ","To":"Laurent Pinchart ","Date":"Thu, 09 Sep 2021 09:04:09 +0200","In-Reply-To":"","References":"<20210902064416.6342-1-martin.kepplinger@puri.sm>\n\t\n\t\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.38.3-1 ","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: add hi846 camera sensor\n\tproperties","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\" "}}]