[{"id":5168,"web_url":"https://patchwork.libcamera.org/comment/5168/","msgid":"<20200610144407.GH192296@oden.dyn.berto.se>","date":"2020-06-10T14:44:07","subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote:\n> Use the new pixel format constants to replace usage of macros from\n> drm_fourcc.h.\n> \n> The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific\n> formats that are not defined in the libcamera public API.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v2:\n> \n> - Drop include drm_fourcc.h from IPU3 pipeline handler\n> \n> Changes since v1:\n> \n> - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++--------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++-----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++---------\n> src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++-----\n> 4 files changed, 30 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index b805fea71c2d..f0428b1baed4 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3)\n> class IPU3CameraData;\n> \n> static const std::map sensorMbusToPixel = {\n> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> };\n> \n> class ImgUDevice\n> @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> {\n> \t/* The only pixel format the driver supports is NV12. */\n> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> +\tcfg.pixelFormat = formats::NV12;\n> \n> \tif (scale) {\n> \t\t/*\n> @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\tconst Size size = cfg.size;\n> \t\tconst IPU3Stream *stream;\n> \n> -\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> +\t\tif (cfg.pixelFormat.modifier())\n\nI wonder if this will work, with this the raw stream would be picked for \nany format with a modifier. I'm thinking about applications erroneously \nconfiguring the raw stream for CSI2 packed layout instead of IPU3 and \nthe validate would accept it.\n\nI think we need to keep the drm_fourcc dependency here verify the \nmodifier is the one for IPU3 layout.\n\n> \t\t\tstream = &data_->rawStream_;\n> \t\telse if (cfg.size == sensorFormat_.size)\n> \t\t\tstream = &data_->outStream_;\n> @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\tStreamConfiguration cfg = {};\n> \t\tIPU3Stream *stream = nullptr;\n> \n> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> +\t\tcfg.pixelFormat = formats::NV12;\n> \n> \t\tswitch (role) {\n> \t\tcase StreamRole::StillCapture:\n> @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> \t\treturn 0;\n> \n> \t*outputFormat = {};\n> -\toutputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12));\n> +\toutputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12);\n> \toutputFormat->size = cfg.size;\n> \toutputFormat->planesCount = 2;\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7f23666ea8f4..60985b716833 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -13,12 +13,12 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> \n> -#include \n> #include \n> \n> #include \"libcamera/internal/camera_sensor.h\"\n> @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \n> \t\tif (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {\n> \t\t\t/* If we cannot find a native format, use a default one. */\n> -\t\t\tcfgPixFmt = PixelFormat(DRM_FORMAT_NV12);\n> +\t\t\tcfgPixFmt = formats::NV12;\n> \t\t\tstatus = Adjusted;\n> \t\t}\n> \t}\n> @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::StillCapture:\n> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> \t\t\t/* Return the largest sensor resolution. */\n> \t\t\tcfg.size = data->sensor_->resolution();\n> \t\t\tcfg.bufferCount = 1;\n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::VideoRecording:\n> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> \t\t\tcfg.size = { 1920, 1080 };\n> \t\t\tcfg.bufferCount = 4;\n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::Viewfinder:\n> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> +\t\t\tcfg.pixelFormat = formats::ARGB8888;\n> \t\t\tcfg.size = { 800, 600 };\n> \t\t\tcfg.bufferCount = 4;\n> \t\t\tbreak;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 900f873ab028..e439f3149bce 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tstatic const std::array formats{\n> -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> -\t\tPixelFormat(DRM_FORMAT_NV16),\n> -\t\tPixelFormat(DRM_FORMAT_NV61),\n> -\t\tPixelFormat(DRM_FORMAT_NV21),\n> -\t\tPixelFormat(DRM_FORMAT_NV12),\n> +\t\tformats::YUYV,\n> +\t\tformats::YVYU,\n> +\t\tformats::VYUY,\n> +\t\tformats::NV16,\n> +\t\tformats::NV61,\n> +\t\tformats::NV21,\n> +\t\tformats::NV12,\n> \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> \t};\n> \n> @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> \t formats.end()) {\n> \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),\n> +\t\tcfg.pixelFormat = formats::NV12,\n> \t\tstatus = Adjusted;\n> \t}\n> \n> @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> \t\treturn config;\n> \n> \tStreamConfiguration cfg{};\n> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> +\tcfg.pixelFormat = formats::NV12;\n> \tcfg.size = data->sensor_->resolution();\n> \n> \tconfig->addConfiguration(cfg);\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 3881545b8a53..b6530662a9ba 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -108,8 +109,8 @@ private:\n> namespace {\n> \n> static const std::map pixelformats{\n> -\t{ PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 },\n> -\t{ PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 },\n> +\t{ formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 },\n> +\t{ formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },\n> };\n> \n> } /* namespace */\n> @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> \tconst std::vector formats = cfg.formats().pixelformats();\n> \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) {\n> \t\tLOG(VIMC, Debug) << \"Adjusting format to BGR888\";\n> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> +\t\tcfg.pixelFormat = formats::BGR888;\n> \t\tstatus = Adjusted;\n> \t}\n> \n> @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> \t\t * but it isn't functional within the pipeline.\n> \t\t */\n> \t\tif (data->media_->version() < KERNEL_VERSION(5, 7, 0)) {\n> -\t\t\tif (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) {\n> +\t\t\tif (pixelformat.first != formats::BGR888) {\n> \t\t\t\tLOG(VIMC, Info)\n> \t\t\t\t\t<< \"Skipping unsupported pixel format \"\n> \t\t\t\t\t<< pixelformat.first.toString();\n> @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> \n> \tStreamConfiguration cfg(formats);\n> \n> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> +\tcfg.pixelFormat = formats::BGR888;\n> \tcfg.size = { 1920, 1080 };\n> \tcfg.bufferCount = 4;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0AFBD600F7\n\tfor ;\n\tWed, 10 Jun 2020 16:44:10 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id a25so2822227ljp.3\n\tfor ;\n\tWed, 10 Jun 2020 07:44:10 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tt4sm502631ljc.28.2020.06.10.07.44.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 10 Jun 2020 07:44:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"IC6yHQaa\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=qpj5vu9QxZtg5lBWjzwkHcuxiaVt3tCEi/DBL6S6GjA=;\n\tb=IC6yHQaaRT4Wn7f+tkQYHJszp7p1czJ7dxvQEKPHuBtHNw9kw89rr3Sc9n24TP3VXh\n\tHOq/D1Uxb59HCmPSL7b5WWlWe/6XQCLyGxGHx5hzohqJ9p7qdG7OROeqKpP2BFxfqZK1\n\tZgt0NNTr+pBytz9iHsxyZ3lvIlNrUVaxt13v007G645navKIUY7jQzbRuDZmNdJdJAFB\n\tsgSLPBZS0GWqY1gg8vcJu7lav/6gehJM9p89fNz2sAbECw+VHQbZj2y9Q5Zs462ddg6c\n\t4jf43W7KiSn5CMYjV2FdKKmRACehopBkLSDGC6JX2fTfPfhOGjee6tpmR1Sa3OCPQbob\n\til5w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=qpj5vu9QxZtg5lBWjzwkHcuxiaVt3tCEi/DBL6S6GjA=;\n\tb=FE3IkDHR/6TjJAX9NE+PmnezWQaZMD1G4G3bbW5DWEP8N7DXKVvDllfn+T71b/Y6BR\n\tTAg0MKOcN2MdmKZozNnzJ5UqAxhPXgYTIi6NmYEzRbotz0Ra4k75ZhiVG9Ocj7dxbxOy\n\tYOFjIVVI73R9k7Cu4V494WERKXRlKImBXaE2qPT+ulYVub1qUF3JPFvwUVY0hA8Vd3aA\n\tsotm3CVX4RY4ytzJtPREN+Mze4mozfFbuPvfUb6MJYaphHxVFKiie/jtvEGe9KKG6+6H\n\tfibKNFEkPQwg4doJ3+QjHI5d4Vh/jw7PnIcoWAsGuGnVbwMzNHUQ11fKUOycX8z7NnUH\n\tSgvA==","X-Gm-Message-State":"AOAM532yCC4kQX54yJ0R43nTpDbpzcbzpL/bhk4yE9UycBdommtLuFrI\n\tpLfya8XVIJXaperfYoAeAgg+N//mu4E=","X-Google-Smtp-Source":"ABdhPJx8p3XqC3FuTCY7mCUD3gNQSNF6Dfl+LaStv6E+OEiS19RfTvgcsNOfCLyTW9iOhC73tQrR7g==","X-Received":"by 2002:a2e:8593:: with SMTP id b19mr1911908lji.6.1591800249279; \n\tWed, 10 Jun 2020 07:44:09 -0700 (PDT)","Date":"Wed, 10 Jun 2020 16:44:07 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200610144407.GH192296@oden.dyn.berto.se>","References":"<20200609232323.29628-7-laurent.pinchart@ideasonboard.com>\n\t<20200610130339.22998-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200610130339.22998-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","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","X-List-Received-Date":"Wed, 10 Jun 2020 14:44:10 -0000"}},{"id":5214,"web_url":"https://patchwork.libcamera.org/comment/5214/","msgid":"<20200616024217.GD29596@pendragon.ideasonboard.com>","date":"2020-06-16T02:42:17","subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote:\n> On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote:\n> > Use the new pixel format constants to replace usage of macros from\n> > drm_fourcc.h.\n> > \n> > The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific\n> > formats that are not defined in the libcamera public API.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v2:\n> > \n> > - Drop include drm_fourcc.h from IPU3 pipeline handler\n> > \n> > Changes since v1:\n> > \n> > - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler\n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++--------\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++-----\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++---------\n> > src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++-----\n> > 4 files changed, 30 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index b805fea71c2d..f0428b1baed4 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -14,6 +14,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3)\n> > class IPU3CameraData;\n> > \n> > static const std::map sensorMbusToPixel = {\n> > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> > };\n> > \n> > class ImgUDevice\n> > @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> > void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> > {\n> > \t/* The only pixel format the driver supports is NV12. */\n> > -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > +\tcfg.pixelFormat = formats::NV12;\n> > \n> > \tif (scale) {\n> > \t\t/*\n> > @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \t\tconst Size size = cfg.size;\n> > \t\tconst IPU3Stream *stream;\n> > \n> > -\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> > +\t\tif (cfg.pixelFormat.modifier())\n> \n> I wonder if this will work, with this the raw stream would be picked for \n> any format with a modifier. I'm thinking about applications erroneously \n> configuring the raw stream for CSI2 packed layout instead of IPU3 and \n> the validate would accept it.\n\nWe have this code further down in the function:\n\n if (stream->raw_) {\n const auto &itFormat =\n sensorMbusToPixel.find(sensorFormat_.mbus_code);\n if (itFormat == sensorMbusToPixel.end())\n return Invalid;\n\n cfg.pixelFormat = itFormat->second;\n cfg.size = sensorFormat_.size;\n }\n\nI think validate() thus correctly updates the pixel format. Is there\nsomething I'm missing ?\n\n> I think we need to keep the drm_fourcc dependency here verify the \n> modifier is the one for IPU3 layout.\n\nWhat should happen if it's not ? Isn't is better to map a CSI-2 packed\nformat, even if not supported, to the raw stream, than to the viewfinder\nor output stream as done today ?\n\nAnother option would be to check the fourcc value to see if it matches\none of the bayer formats, and ignoring the modifier.\n\n> > \t\t\tstream = &data_->rawStream_;\n> > \t\telse if (cfg.size == sensorFormat_.size)\n> > \t\t\tstream = &data_->outStream_;\n> > @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > \t\tStreamConfiguration cfg = {};\n> > \t\tIPU3Stream *stream = nullptr;\n> > \n> > -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > +\t\tcfg.pixelFormat = formats::NV12;\n> > \n> > \t\tswitch (role) {\n> > \t\tcase StreamRole::StillCapture:\n> > @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > \t\treturn 0;\n> > \n> > \t*outputFormat = {};\n> > -\toutputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12));\n> > +\toutputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12);\n> > \toutputFormat->size = cfg.size;\n> > \toutputFormat->planesCount = 2;\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7f23666ea8f4..60985b716833 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -13,12 +13,12 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > #include \n> > \n> > -#include \n> > #include \n> > \n> > #include \"libcamera/internal/camera_sensor.h\"\n> > @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > \n> > \t\tif (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {\n> > \t\t\t/* If we cannot find a native format, use a default one. */\n> > -\t\t\tcfgPixFmt = PixelFormat(DRM_FORMAT_NV12);\n> > +\t\t\tcfgPixFmt = formats::NV12;\n> > \t\t\tstatus = Adjusted;\n> > \t\t}\n> > \t}\n> > @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > \t\t\tbreak;\n> > \n> > \t\tcase StreamRole::StillCapture:\n> > -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > \t\t\t/* Return the largest sensor resolution. */\n> > \t\t\tcfg.size = data->sensor_->resolution();\n> > \t\t\tcfg.bufferCount = 1;\n> > \t\t\tbreak;\n> > \n> > \t\tcase StreamRole::VideoRecording:\n> > -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > \t\t\tcfg.size = { 1920, 1080 };\n> > \t\t\tcfg.bufferCount = 4;\n> > \t\t\tbreak;\n> > \n> > \t\tcase StreamRole::Viewfinder:\n> > -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > +\t\t\tcfg.pixelFormat = formats::ARGB8888;\n> > \t\t\tcfg.size = { 800, 600 };\n> > \t\t\tcfg.bufferCount = 4;\n> > \t\t\tbreak;\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 900f873ab028..e439f3149bce 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > {\n> > \tstatic const std::array formats{\n> > -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> > -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> > -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> > -\t\tPixelFormat(DRM_FORMAT_NV16),\n> > -\t\tPixelFormat(DRM_FORMAT_NV61),\n> > -\t\tPixelFormat(DRM_FORMAT_NV21),\n> > -\t\tPixelFormat(DRM_FORMAT_NV12),\n> > +\t\tformats::YUYV,\n> > +\t\tformats::YVYU,\n> > +\t\tformats::VYUY,\n> > +\t\tformats::NV16,\n> > +\t\tformats::NV61,\n> > +\t\tformats::NV21,\n> > +\t\tformats::NV12,\n> > \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > \t};\n> > \n> > @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> > \t formats.end()) {\n> > \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),\n> > +\t\tcfg.pixelFormat = formats::NV12,\n> > \t\tstatus = Adjusted;\n> > \t}\n> > \n> > @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > \t\treturn config;\n> > \n> > \tStreamConfiguration cfg{};\n> > -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > +\tcfg.pixelFormat = formats::NV12;\n> > \tcfg.size = data->sensor_->resolution();\n> > \n> > \tconfig->addConfiguration(cfg);\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 3881545b8a53..b6530662a9ba 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -17,6 +17,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -108,8 +109,8 @@ private:\n> > namespace {\n> > \n> > static const std::map pixelformats{\n> > -\t{ PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 },\n> > -\t{ PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 },\n> > +\t{ formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 },\n> > +\t{ formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },\n> > };\n> > \n> > } /* namespace */\n> > @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > \tconst std::vector formats = cfg.formats().pixelformats();\n> > \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) {\n> > \t\tLOG(VIMC, Debug) << \"Adjusting format to BGR888\";\n> > -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > +\t\tcfg.pixelFormat = formats::BGR888;\n> > \t\tstatus = Adjusted;\n> > \t}\n> > \n> > @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > \t\t * but it isn't functional within the pipeline.\n> > \t\t */\n> > \t\tif (data->media_->version() < KERNEL_VERSION(5, 7, 0)) {\n> > -\t\t\tif (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) {\n> > +\t\t\tif (pixelformat.first != formats::BGR888) {\n> > \t\t\t\tLOG(VIMC, Info)\n> > \t\t\t\t\t<< \"Skipping unsupported pixel format \"\n> > \t\t\t\t\t<< pixelformat.first.toString();\n> > @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > \n> > \tStreamConfiguration cfg(formats);\n> > \n> > -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > +\tcfg.pixelFormat = formats::BGR888;\n> > \tcfg.size = { 1920, 1080 };\n> > \tcfg.bufferCount = 4;\n> >","headers":{"Return-Path":"","Received":["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 F3B4961027\n\tfor ;\n\tTue, 16 Jun 2020 04:42:39 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73403F9;\n\tTue, 16 Jun 2020 04:42:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pj511D+H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592275359;\n\tbh=TvRa05YDRQjRa4gN9bMHkan0XleeB4j3Yqicb64rbeA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pj511D+Ht5Yri4WLTpCDz+BuiGooqa/XLSCNyNx7BeNhR0ziJ8Fi5DWwqfDMSztxO\n\t0UuIVljuS296IotsOj6sKub/2tNDotDv3h2A3Bu9dbpcMxAsOG6sqER2V8XceyP9Ki\n\twsxoEMZykih0Uoanoo7efipNajL2AKago5kp9+Xk=","Date":"Tue, 16 Jun 2020 05:42:17 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200616024217.GD29596@pendragon.ideasonboard.com>","References":"<20200609232323.29628-7-laurent.pinchart@ideasonboard.com>\n\t<20200610130339.22998-1-laurent.pinchart@ideasonboard.com>\n\t<20200610144407.GH192296@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200610144407.GH192296@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","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","X-List-Received-Date":"Tue, 16 Jun 2020 02:42:40 -0000"}},{"id":5218,"web_url":"https://patchwork.libcamera.org/comment/5218/","msgid":"<20200616152952.GA2565716@oden.dyn.berto.se>","date":"2020-06-16T15:29:52","subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2020-06-16 05:42:17 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote:\n> > On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote:\n> > > Use the new pixel format constants to replace usage of macros from\n> > > drm_fourcc.h.\n> > > \n> > > The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific\n> > > formats that are not defined in the libcamera public API.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Changes since v2:\n> > > \n> > > - Drop include drm_fourcc.h from IPU3 pipeline handler\n> > > \n> > > Changes since v1:\n> > > \n> > > - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler\n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++--------\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++-----\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++---------\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++-----\n> > > 4 files changed, 30 insertions(+), 27 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index b805fea71c2d..f0428b1baed4 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -14,6 +14,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > \n> > > @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3)\n> > > class IPU3CameraData;\n> > > \n> > > static const std::map sensorMbusToPixel = {\n> > > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> > > };\n> > > \n> > > class ImgUDevice\n> > > @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> > > void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> > > {\n> > > \t/* The only pixel format the driver supports is NV12. */\n> > > -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > +\tcfg.pixelFormat = formats::NV12;\n> > > \n> > > \tif (scale) {\n> > > \t\t/*\n> > > @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > \t\tconst Size size = cfg.size;\n> > > \t\tconst IPU3Stream *stream;\n> > > \n> > > -\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> > > +\t\tif (cfg.pixelFormat.modifier())\n> > \n> > I wonder if this will work, with this the raw stream would be picked for \n> > any format with a modifier. I'm thinking about applications erroneously \n> > configuring the raw stream for CSI2 packed layout instead of IPU3 and \n> > the validate would accept it.\n> \n> We have this code further down in the function:\n> \n> if (stream->raw_) {\n> const auto &itFormat =\n> sensorMbusToPixel.find(sensorFormat_.mbus_code);\n> if (itFormat == sensorMbusToPixel.end())\n> return Invalid;\n> \n> cfg.pixelFormat = itFormat->second;\n> cfg.size = sensorFormat_.size;\n> }\n\nYou are correct, thanks for pointing this out. Please add my,\n\nReviewed-by: Niklas Söderlund \n\n> \n> I think validate() thus correctly updates the pixel format. Is there\n> something I'm missing ?\n> \n> > I think we need to keep the drm_fourcc dependency here verify the \n> > modifier is the one for IPU3 layout.\n> \n> What should happen if it's not ? Isn't is better to map a CSI-2 packed\n> format, even if not supported, to the raw stream, than to the viewfinder\n> or output stream as done today ?\n> \n> Another option would be to check the fourcc value to see if it matches\n> one of the bayer formats, and ignoring the modifier.\n> \n> > > \t\t\tstream = &data_->rawStream_;\n> > > \t\telse if (cfg.size == sensorFormat_.size)\n> > > \t\t\tstream = &data_->outStream_;\n> > > @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > \t\tStreamConfiguration cfg = {};\n> > > \t\tIPU3Stream *stream = nullptr;\n> > > \n> > > -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > +\t\tcfg.pixelFormat = formats::NV12;\n> > > \n> > > \t\tswitch (role) {\n> > > \t\tcase StreamRole::StillCapture:\n> > > @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > > \t\treturn 0;\n> > > \n> > > \t*outputFormat = {};\n> > > -\toutputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12));\n> > > +\toutputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12);\n> > > \toutputFormat->size = cfg.size;\n> > > \toutputFormat->planesCount = 2;\n> > > \n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 7f23666ea8f4..60985b716833 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -13,12 +13,12 @@\n> > > \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > #include \n> > > \n> > > -#include \n> > > #include \n> > > \n> > > #include \"libcamera/internal/camera_sensor.h\"\n> > > @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > \n> > > \t\tif (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {\n> > > \t\t\t/* If we cannot find a native format, use a default one. */\n> > > -\t\t\tcfgPixFmt = PixelFormat(DRM_FORMAT_NV12);\n> > > +\t\t\tcfgPixFmt = formats::NV12;\n> > > \t\t\tstatus = Adjusted;\n> > > \t\t}\n> > > \t}\n> > > @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > \t\t\tbreak;\n> > > \n> > > \t\tcase StreamRole::StillCapture:\n> > > -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > > \t\t\t/* Return the largest sensor resolution. */\n> > > \t\t\tcfg.size = data->sensor_->resolution();\n> > > \t\t\tcfg.bufferCount = 1;\n> > > \t\t\tbreak;\n> > > \n> > > \t\tcase StreamRole::VideoRecording:\n> > > -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > > \t\t\tcfg.size = { 1920, 1080 };\n> > > \t\t\tcfg.bufferCount = 4;\n> > > \t\t\tbreak;\n> > > \n> > > \t\tcase StreamRole::Viewfinder:\n> > > -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > > +\t\t\tcfg.pixelFormat = formats::ARGB8888;\n> > > \t\t\tcfg.size = { 800, 600 };\n> > > \t\t\tcfg.bufferCount = 4;\n> > > \t\t\tbreak;\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 900f873ab028..e439f3149bce 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -16,6 +16,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > {\n> > > \tstatic const std::array formats{\n> > > -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> > > -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> > > -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> > > -\t\tPixelFormat(DRM_FORMAT_NV16),\n> > > -\t\tPixelFormat(DRM_FORMAT_NV61),\n> > > -\t\tPixelFormat(DRM_FORMAT_NV21),\n> > > -\t\tPixelFormat(DRM_FORMAT_NV12),\n> > > +\t\tformats::YUYV,\n> > > +\t\tformats::YVYU,\n> > > +\t\tformats::VYUY,\n> > > +\t\tformats::NV16,\n> > > +\t\tformats::NV61,\n> > > +\t\tformats::NV21,\n> > > +\t\tformats::NV12,\n> > > \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > > \t};\n> > > \n> > > @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> > > \t formats.end()) {\n> > > \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > > -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),\n> > > +\t\tcfg.pixelFormat = formats::NV12,\n> > > \t\tstatus = Adjusted;\n> > > \t}\n> > > \n> > > @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > > \t\treturn config;\n> > > \n> > > \tStreamConfiguration cfg{};\n> > > -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > +\tcfg.pixelFormat = formats::NV12;\n> > > \tcfg.size = data->sensor_->resolution();\n> > > \n> > > \tconfig->addConfiguration(cfg);\n> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > index 3881545b8a53..b6530662a9ba 100644\n> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -108,8 +109,8 @@ private:\n> > > namespace {\n> > > \n> > > static const std::map pixelformats{\n> > > -\t{ PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 },\n> > > -\t{ PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 },\n> > > +\t{ formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 },\n> > > +\t{ formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },\n> > > };\n> > > \n> > > } /* namespace */\n> > > @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > > \tconst std::vector formats = cfg.formats().pixelformats();\n> > > \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) {\n> > > \t\tLOG(VIMC, Debug) << \"Adjusting format to BGR888\";\n> > > -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > > +\t\tcfg.pixelFormat = formats::BGR888;\n> > > \t\tstatus = Adjusted;\n> > > \t}\n> > > \n> > > @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > > \t\t * but it isn't functional within the pipeline.\n> > > \t\t */\n> > > \t\tif (data->media_->version() < KERNEL_VERSION(5, 7, 0)) {\n> > > -\t\t\tif (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) {\n> > > +\t\t\tif (pixelformat.first != formats::BGR888) {\n> > > \t\t\t\tLOG(VIMC, Info)\n> > > \t\t\t\t\t<< \"Skipping unsupported pixel format \"\n> > > \t\t\t\t\t<< pixelformat.first.toString();\n> > > @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > > \n> > > \tStreamConfiguration cfg(formats);\n> > > \n> > > -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > > +\tcfg.pixelFormat = formats::BGR888;\n> > > \tcfg.size = { 1920, 1080 };\n> > > \tcfg.bufferCount = 4;\n> > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62E60603C4\n\tfor ;\n\tTue, 16 Jun 2020 17:29:54 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id z206so12017915lfc.6\n\tfor ;\n\tTue, 16 Jun 2020 08:29:54 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tq13sm409857lfb.55.2020.06.16.08.29.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 16 Jun 2020 08:29:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"QgoyIn3M\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=3PgskmF525AhaQc6TYGpRKDekSbRpNNOOdsni30WlGE=;\n\tb=QgoyIn3MUQJSwXQT+4HGxGaZY7LP9zdPwPZWUf3KHCjwxOdDz2FbA0HRsBT3IJUr7h\n\tpDvZ7gtcrglBuuz2klFXOAkdEnEaqCk/zmL2qjEWCv9WH7XRcToC4+G1sakMayoqA9gv\n\teEr38QSY3WZcCRpymAF0ZSmq5Nq08fb4zEP5wYVDs+IkskV2ienobHpMOZQYAhGKEUZT\n\tPdJnzrZEpYH2QfgYjtDbOFR5n7Y73YGANuiRs/sdtbnqWGmI8xlhXz26HKngea2oJOKI\n\tuZIhAkZk2gtm+7EvUAfm0v9OnCySPdiAxukYSqyONWl/rPR12QN9iQxA76/TqV1npoCJ\n\tNp7Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=3PgskmF525AhaQc6TYGpRKDekSbRpNNOOdsni30WlGE=;\n\tb=k+WJQt+RGAZ0FnrchrZ6wgfOu0mfPb7rjcviqw+rzXSj9yLIAfjbXS7AwpsyUXegD5\n\tMVAcriFAJ2WquoRzcPhRPeDxPdjr/M+EQlVhjQ8LVSB4OoNootxASqDZ6vi1zW0egHIM\n\tOMvjY3GFBI4+YM0Kkawjra3Qk2IlWijXBOkp626Q9zN0AqJhiH4KQ1EAt1tbmro55lJM\n\tucVrjhURlFLdsGZgoeugohVXr0QIfZNvMQXOYGu7iIGhis0eIPaHQQ813HDCf3BM5xiz\n\t8vRMsDPSU8LdY4/Vy7DpujCPbwjLrHeiTZ3FgVns9QEiUnPrLAzkJZvXW4v1PVfMBwoi\n\tTbXg==","X-Gm-Message-State":"AOAM5302jEDBlA/KXmVu14kw+CDG27jagLN4qEB26o0zsTnYO4AXNQAL\n\tVcv177OqTPk7463dsCymykoolnjflXI=","X-Google-Smtp-Source":"ABdhPJzTewT63NcoZZDGb82hgF+4UjxBx1BHn8z4aWW62kVj+R2q1C4KXYf9MJLOo3CEKfRiNLlvhw==","X-Received":"by 2002:a05:6512:318f:: with SMTP id\n\ti15mr590272lfe.203.1592321393372; \n\tTue, 16 Jun 2020 08:29:53 -0700 (PDT)","Date":"Tue, 16 Jun 2020 17:29:52 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200616152952.GA2565716@oden.dyn.berto.se>","References":"<20200609232323.29628-7-laurent.pinchart@ideasonboard.com>\n\t<20200610130339.22998-1-laurent.pinchart@ideasonboard.com>\n\t<20200610144407.GH192296@oden.dyn.berto.se>\n\t<20200616024217.GD29596@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200616024217.GD29596@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","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","X-List-Received-Date":"Tue, 16 Jun 2020 15:29:54 -0000"}},{"id":5222,"web_url":"https://patchwork.libcamera.org/comment/5222/","msgid":"<20200616185506.GF913@pendragon.ideasonboard.com>","date":"2020-06-16T18:55:06","subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Jun 16, 2020 at 05:29:52PM +0200, Niklas Söderlund wrote:\n> On 2020-06-16 05:42:17 +0300, Laurent Pinchart wrote:\n> > On Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote:\n> >> On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote:\n> >>> Use the new pixel format constants to replace usage of macros from\n> >>> drm_fourcc.h.\n> >>> \n> >>> The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific\n> >>> formats that are not defined in the libcamera public API.\n> >>> \n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> Changes since v2:\n> >>> \n> >>> - Drop include drm_fourcc.h from IPU3 pipeline handler\n> >>> \n> >>> Changes since v1:\n> >>> \n> >>> - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler\n> >>> ---\n> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++--------\n> >>> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++-----\n> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++---------\n> >>> src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++-----\n> >>> 4 files changed, 30 insertions(+), 27 deletions(-)\n> >>> \n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index b805fea71c2d..f0428b1baed4 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -14,6 +14,7 @@\n> >>> #include \n> >>> \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> \n> >>> @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3)\n> >>> class IPU3CameraData;\n> >>> \n> >>> static const std::map sensorMbusToPixel = {\n> >>> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> >>> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> >>> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> >>> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> >>> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> >>> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> >>> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> >>> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> >>> };\n> >>> \n> >>> class ImgUDevice\n> >>> @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> >>> void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> >>> {\n> >>> \t/* The only pixel format the driver supports is NV12. */\n> >>> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> +\tcfg.pixelFormat = formats::NV12;\n> >>> \n> >>> \tif (scale) {\n> >>> \t\t/*\n> >>> @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >>> \t\tconst Size size = cfg.size;\n> >>> \t\tconst IPU3Stream *stream;\n> >>> \n> >>> -\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> >>> +\t\tif (cfg.pixelFormat.modifier())\n> >> \n> >> I wonder if this will work, with this the raw stream would be picked for \n> >> any format with a modifier. I'm thinking about applications erroneously \n> >> configuring the raw stream for CSI2 packed layout instead of IPU3 and \n> >> the validate would accept it.\n> > \n> > We have this code further down in the function:\n> > \n> > if (stream->raw_) {\n> > const auto &itFormat =\n> > sensorMbusToPixel.find(sensorFormat_.mbus_code);\n> > if (itFormat == sensorMbusToPixel.end())\n> > return Invalid;\n> > \n> > cfg.pixelFormat = itFormat->second;\n> > cfg.size = sensorFormat_.size;\n> > }\n> \n> You are correct, thanks for pointing this out. Please add my,\n> \n> Reviewed-by: Niklas Söderlund \n\nWould you prefer the following though (to be folded in this patch) ?\n\ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex f0428b1baed4..93faf8e19e29 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -364,10 +364,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n \tfor (unsigned int i = 0; i < config_.size(); ++i) {\n \t\tStreamConfiguration &cfg = config_[i];\n \t\tconst PixelFormat pixelFormat = cfg.pixelFormat;\n+\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n \t\tconst Size size = cfg.size;\n \t\tconst IPU3Stream *stream;\n\n-\t\tif (cfg.pixelFormat.modifier())\n+\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n \t\t\tstream = &data_->rawStream_;\n \t\telse if (cfg.size == sensorFormat_.size)\n \t\t\tstream = &data_->outStream_;\n\nA tad bit more expensive at runtime, but more explicit. Let me know\nwhich version you like best.\n\n> > I think validate() thus correctly updates the pixel format. Is there\n> > something I'm missing ?\n> > \n> >> I think we need to keep the drm_fourcc dependency here verify the \n> >> modifier is the one for IPU3 layout.\n> > \n> > What should happen if it's not ? Isn't is better to map a CSI-2 packed\n> > format, even if not supported, to the raw stream, than to the viewfinder\n> > or output stream as done today ?\n> > \n> > Another option would be to check the fourcc value to see if it matches\n> > one of the bayer formats, and ignoring the modifier.\n> > \n> >>> \t\t\tstream = &data_->rawStream_;\n> >>> \t\telse if (cfg.size == sensorFormat_.size)\n> >>> \t\t\tstream = &data_->outStream_;\n> >>> @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >>> \t\tStreamConfiguration cfg = {};\n> >>> \t\tIPU3Stream *stream = nullptr;\n> >>> \n> >>> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> +\t\tcfg.pixelFormat = formats::NV12;\n> >>> \n> >>> \t\tswitch (role) {\n> >>> \t\tcase StreamRole::StillCapture:\n> >>> @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> >>> \t\treturn 0;\n> >>> \n> >>> \t*outputFormat = {};\n> >>> -\toutputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12));\n> >>> +\toutputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12);\n> >>> \toutputFormat->size = cfg.size;\n> >>> \toutputFormat->planesCount = 2;\n> >>> \n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> index 7f23666ea8f4..60985b716833 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> @@ -13,12 +13,12 @@\n> >>> \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> \n> >>> -#include \n> >>> #include \n> >>> \n> >>> #include \"libcamera/internal/camera_sensor.h\"\n> >>> @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> >>> \n> >>> \t\tif (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {\n> >>> \t\t\t/* If we cannot find a native format, use a default one. */\n> >>> -\t\t\tcfgPixFmt = PixelFormat(DRM_FORMAT_NV12);\n> >>> +\t\t\tcfgPixFmt = formats::NV12;\n> >>> \t\t\tstatus = Adjusted;\n> >>> \t\t}\n> >>> \t}\n> >>> @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>> \t\t\tbreak;\n> >>> \n> >>> \t\tcase StreamRole::StillCapture:\n> >>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> +\t\t\tcfg.pixelFormat = formats::NV12;\n> >>> \t\t\t/* Return the largest sensor resolution. */\n> >>> \t\t\tcfg.size = data->sensor_->resolution();\n> >>> \t\t\tcfg.bufferCount = 1;\n> >>> \t\t\tbreak;\n> >>> \n> >>> \t\tcase StreamRole::VideoRecording:\n> >>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> +\t\t\tcfg.pixelFormat = formats::NV12;\n> >>> \t\t\tcfg.size = { 1920, 1080 };\n> >>> \t\t\tcfg.bufferCount = 4;\n> >>> \t\t\tbreak;\n> >>> \n> >>> \t\tcase StreamRole::Viewfinder:\n> >>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>> +\t\t\tcfg.pixelFormat = formats::ARGB8888;\n> >>> \t\t\tcfg.size = { 800, 600 };\n> >>> \t\t\tcfg.bufferCount = 4;\n> >>> \t\t\tbreak;\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> index 900f873ab028..e439f3149bce 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> @@ -16,6 +16,7 @@\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> >>> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>> {\n> >>> \tstatic const std::array formats{\n> >>> -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> >>> -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> >>> -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV16),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV61),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV21),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV12),\n> >>> +\t\tformats::YUYV,\n> >>> +\t\tformats::YVYU,\n> >>> +\t\tformats::VYUY,\n> >>> +\t\tformats::NV16,\n> >>> +\t\tformats::NV61,\n> >>> +\t\tformats::NV21,\n> >>> +\t\tformats::NV12,\n> >>> \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> >>> \t};\n> >>> \n> >>> @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>> \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> >>> \t formats.end()) {\n> >>> \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> >>> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),\n> >>> +\t\tcfg.pixelFormat = formats::NV12,\n> >>> \t\tstatus = Adjusted;\n> >>> \t}\n> >>> \n> >>> @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> >>> \t\treturn config;\n> >>> \n> >>> \tStreamConfiguration cfg{};\n> >>> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> +\tcfg.pixelFormat = formats::NV12;\n> >>> \tcfg.size = data->sensor_->resolution();\n> >>> \n> >>> \tconfig->addConfiguration(cfg);\n> >>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> >>> index 3881545b8a53..b6530662a9ba 100644\n> >>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> >>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> >>> @@ -17,6 +17,7 @@\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -108,8 +109,8 @@ private:\n> >>> namespace {\n> >>> \n> >>> static const std::map pixelformats{\n> >>> -\t{ PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 },\n> >>> -\t{ PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 },\n> >>> +\t{ formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 },\n> >>> +\t{ formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },\n> >>> };\n> >>> \n> >>> } /* namespace */\n> >>> @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> >>> \tconst std::vector formats = cfg.formats().pixelformats();\n> >>> \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) {\n> >>> \t\tLOG(VIMC, Debug) << \"Adjusting format to BGR888\";\n> >>> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> >>> +\t\tcfg.pixelFormat = formats::BGR888;\n> >>> \t\tstatus = Adjusted;\n> >>> \t}\n> >>> \n> >>> @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> >>> \t\t * but it isn't functional within the pipeline.\n> >>> \t\t */\n> >>> \t\tif (data->media_->version() < KERNEL_VERSION(5, 7, 0)) {\n> >>> -\t\t\tif (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) {\n> >>> +\t\t\tif (pixelformat.first != formats::BGR888) {\n> >>> \t\t\t\tLOG(VIMC, Info)\n> >>> \t\t\t\t\t<< \"Skipping unsupported pixel format \"\n> >>> \t\t\t\t\t<< pixelformat.first.toString();\n> >>> @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> >>> \n> >>> \tStreamConfiguration cfg(formats);\n> >>> \n> >>> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> >>> +\tcfg.pixelFormat = formats::BGR888;\n> >>> \tcfg.size = { 1920, 1080 };\n> >>> \tcfg.bufferCount = 4;\n> >>>","headers":{"Return-Path":"","Received":["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 E822A603C4\n\tfor ;\n\tTue, 16 Jun 2020 20:55:29 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5F593F9;\n\tTue, 16 Jun 2020 20:55:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hCaQxJeL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592333729;\n\tbh=fciCM7DHjS6KyHY7rJf2C/1xYVfZ6NX5zcafcX1zfvA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hCaQxJeLlctNBw5vwkXzz6uBXCAsw8URfuBK3xDd9ceSCuC/FKOy1vN4GDCXyLQV9\n\tL1+maDYe+0PhBL9tUt8Jlp/0vB4uCC/LoW7EBNex8URbTHXOhbFhokXR+CN4ODbcKg\n\tcsLBdSnDlDBedbovbOeBJCf6SU/cGSvBfNoVIZ20=","Date":"Tue, 16 Jun 2020 21:55:06 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200616185506.GF913@pendragon.ideasonboard.com>","References":"<20200609232323.29628-7-laurent.pinchart@ideasonboard.com>\n\t<20200610130339.22998-1-laurent.pinchart@ideasonboard.com>\n\t<20200610144407.GH192296@oden.dyn.berto.se>\n\t<20200616024217.GD29596@pendragon.ideasonboard.com>\n\t<20200616152952.GA2565716@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200616152952.GA2565716@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","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","X-List-Received-Date":"Tue, 16 Jun 2020 18:55:30 -0000"}},{"id":5235,"web_url":"https://patchwork.libcamera.org/comment/5235/","msgid":"<20200617093213.GA2850317@oden.dyn.berto.se>","date":"2020-06-17T09:32:13","subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2020-06-16 21:55:06 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Tue, Jun 16, 2020 at 05:29:52PM +0200, Niklas Söderlund wrote:\n> > On 2020-06-16 05:42:17 +0300, Laurent Pinchart wrote:\n> > > On Wed, Jun 10, 2020 at 04:44:07PM +0200, Niklas Söderlund wrote:\n> > >> On 2020-06-10 16:03:39 +0300, Laurent Pinchart wrote:\n> > >>> Use the new pixel format constants to replace usage of macros from\n> > >>> drm_fourcc.h.\n> > >>> \n> > >>> The IPU3 pipeline handler still uses DRM FourCCs for IPU3-specific\n> > >>> formats that are not defined in the libcamera public API.\n> > >>> \n> > >>> Signed-off-by: Laurent Pinchart \n> > >>> ---\n> > >>> Changes since v2:\n> > >>> \n> > >>> - Drop include drm_fourcc.h from IPU3 pipeline handler\n> > >>> \n> > >>> Changes since v1:\n> > >>> \n> > >>> - Use formats::S[RGB]{4}_IPU3 in the IPU3 pipeline handler\n> > >>> ---\n> > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++--------\n> > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++++-----\n> > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++---------\n> > >>> src/libcamera/pipeline/vimc/vimc.cpp | 11 ++++++-----\n> > >>> 4 files changed, 30 insertions(+), 27 deletions(-)\n> > >>> \n> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> index b805fea71c2d..f0428b1baed4 100644\n> > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> @@ -14,6 +14,7 @@\n> > >>> #include \n> > >>> \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> \n> > >>> @@ -34,10 +35,10 @@ LOG_DEFINE_CATEGORY(IPU3)\n> > >>> class IPU3CameraData;\n> > >>> \n> > >>> static const std::map sensorMbusToPixel = {\n> > >>> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > >>> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > >>> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > >>> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > >>> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> > >>> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> > >>> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> > >>> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> > >>> };\n> > >>> \n> > >>> class ImgUDevice\n> > >>> @@ -261,7 +262,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> > >>> void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> > >>> {\n> > >>> \t/* The only pixel format the driver supports is NV12. */\n> > >>> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> +\tcfg.pixelFormat = formats::NV12;\n> > >>> \n> > >>> \tif (scale) {\n> > >>> \t\t/*\n> > >>> @@ -366,7 +367,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > >>> \t\tconst Size size = cfg.size;\n> > >>> \t\tconst IPU3Stream *stream;\n> > >>> \n> > >>> -\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> > >>> +\t\tif (cfg.pixelFormat.modifier())\n> > >> \n> > >> I wonder if this will work, with this the raw stream would be picked for \n> > >> any format with a modifier. I'm thinking about applications erroneously \n> > >> configuring the raw stream for CSI2 packed layout instead of IPU3 and \n> > >> the validate would accept it.\n> > > \n> > > We have this code further down in the function:\n> > > \n> > > if (stream->raw_) {\n> > > const auto &itFormat =\n> > > sensorMbusToPixel.find(sensorFormat_.mbus_code);\n> > > if (itFormat == sensorMbusToPixel.end())\n> > > return Invalid;\n> > > \n> > > cfg.pixelFormat = itFormat->second;\n> > > cfg.size = sensorFormat_.size;\n> > > }\n> > \n> > You are correct, thanks for pointing this out. Please add my,\n> > \n> > Reviewed-by: Niklas Söderlund \n> \n> Would you prefer the following though (to be folded in this patch) ?\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index f0428b1baed4..93faf8e19e29 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -364,10 +364,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \tfor (unsigned int i = 0; i < config_.size(); ++i) {\n> \t\tStreamConfiguration &cfg = config_[i];\n> \t\tconst PixelFormat pixelFormat = cfg.pixelFormat;\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n> \t\tconst Size size = cfg.size;\n> \t\tconst IPU3Stream *stream;\n> \n> -\t\tif (cfg.pixelFormat.modifier())\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> \t\t\tstream = &data_->rawStream_;\n> \t\telse if (cfg.size == sensorFormat_.size)\n> \t\t\tstream = &data_->outStream_;\n> \n> A tad bit more expensive at runtime, but more explicit. Let me know\n> which version you like best.\n\nI like this changed. Both versions are OK with me with this extra twist \ngaining bonus points. If you squash it in feel free to keep my R-b.\n\n> \n> > > I think validate() thus correctly updates the pixel format. Is there\n> > > something I'm missing ?\n> > > \n> > >> I think we need to keep the drm_fourcc dependency here verify the \n> > >> modifier is the one for IPU3 layout.\n> > > \n> > > What should happen if it's not ? Isn't is better to map a CSI-2 packed\n> > > format, even if not supported, to the raw stream, than to the viewfinder\n> > > or output stream as done today ?\n> > > \n> > > Another option would be to check the fourcc value to see if it matches\n> > > one of the bayer formats, and ignoring the modifier.\n> > > \n> > >>> \t\t\tstream = &data_->rawStream_;\n> > >>> \t\telse if (cfg.size == sensorFormat_.size)\n> > >>> \t\t\tstream = &data_->outStream_;\n> > >>> @@ -430,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > >>> \t\tStreamConfiguration cfg = {};\n> > >>> \t\tIPU3Stream *stream = nullptr;\n> > >>> \n> > >>> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> +\t\tcfg.pixelFormat = formats::NV12;\n> > >>> \n> > >>> \t\tswitch (role) {\n> > >>> \t\tcase StreamRole::StillCapture:\n> > >>> @@ -1193,7 +1194,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > >>> \t\treturn 0;\n> > >>> \n> > >>> \t*outputFormat = {};\n> > >>> -\toutputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12));\n> > >>> +\toutputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12);\n> > >>> \toutputFormat->size = cfg.size;\n> > >>> \toutputFormat->planesCount = 2;\n> > >>> \n> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> index 7f23666ea8f4..60985b716833 100644\n> > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> @@ -13,12 +13,12 @@\n> > >>> \n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> \n> > >>> -#include \n> > >>> #include \n> > >>> \n> > >>> #include \"libcamera/internal/camera_sensor.h\"\n> > >>> @@ -490,7 +490,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > >>> \n> > >>> \t\tif (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {\n> > >>> \t\t\t/* If we cannot find a native format, use a default one. */\n> > >>> -\t\t\tcfgPixFmt = PixelFormat(DRM_FORMAT_NV12);\n> > >>> +\t\t\tcfgPixFmt = formats::NV12;\n> > >>> \t\t\tstatus = Adjusted;\n> > >>> \t\t}\n> > >>> \t}\n> > >>> @@ -537,20 +537,20 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >>> \t\t\tbreak;\n> > >>> \n> > >>> \t\tcase StreamRole::StillCapture:\n> > >>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> +\t\t\tcfg.pixelFormat = formats::NV12;\n> > >>> \t\t\t/* Return the largest sensor resolution. */\n> > >>> \t\t\tcfg.size = data->sensor_->resolution();\n> > >>> \t\t\tcfg.bufferCount = 1;\n> > >>> \t\t\tbreak;\n> > >>> \n> > >>> \t\tcase StreamRole::VideoRecording:\n> > >>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> +\t\t\tcfg.pixelFormat = formats::NV12;\n> > >>> \t\t\tcfg.size = { 1920, 1080 };\n> > >>> \t\t\tcfg.bufferCount = 4;\n> > >>> \t\t\tbreak;\n> > >>> \n> > >>> \t\tcase StreamRole::Viewfinder:\n> > >>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > >>> +\t\t\tcfg.pixelFormat = formats::ARGB8888;\n> > >>> \t\t\tcfg.size = { 800, 600 };\n> > >>> \t\t\tcfg.bufferCount = 4;\n> > >>> \t\t\tbreak;\n> > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> index 900f873ab028..e439f3149bce 100644\n> > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> @@ -16,6 +16,7 @@\n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> @@ -459,13 +460,13 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > >>> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >>> {\n> > >>> \tstatic const std::array formats{\n> > >>> -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> > >>> -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> > >>> -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> > >>> -\t\tPixelFormat(DRM_FORMAT_NV16),\n> > >>> -\t\tPixelFormat(DRM_FORMAT_NV61),\n> > >>> -\t\tPixelFormat(DRM_FORMAT_NV21),\n> > >>> -\t\tPixelFormat(DRM_FORMAT_NV12),\n> > >>> +\t\tformats::YUYV,\n> > >>> +\t\tformats::YVYU,\n> > >>> +\t\tformats::VYUY,\n> > >>> +\t\tformats::NV16,\n> > >>> +\t\tformats::NV61,\n> > >>> +\t\tformats::NV21,\n> > >>> +\t\tformats::NV12,\n> > >>> \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > >>> \t};\n> > >>> \n> > >>> @@ -487,7 +488,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >>> \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> > >>> \t formats.end()) {\n> > >>> \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > >>> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),\n> > >>> +\t\tcfg.pixelFormat = formats::NV12,\n> > >>> \t\tstatus = Adjusted;\n> > >>> \t}\n> > >>> \n> > >>> @@ -566,7 +567,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > >>> \t\treturn config;\n> > >>> \n> > >>> \tStreamConfiguration cfg{};\n> > >>> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> +\tcfg.pixelFormat = formats::NV12;\n> > >>> \tcfg.size = data->sensor_->resolution();\n> > >>> \n> > >>> \tconfig->addConfiguration(cfg);\n> > >>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > >>> index 3881545b8a53..b6530662a9ba 100644\n> > >>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > >>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > >>> @@ -17,6 +17,7 @@\n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> @@ -108,8 +109,8 @@ private:\n> > >>> namespace {\n> > >>> \n> > >>> static const std::map pixelformats{\n> > >>> -\t{ PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 },\n> > >>> -\t{ PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 },\n> > >>> +\t{ formats::RGB888, MEDIA_BUS_FMT_BGR888_1X24 },\n> > >>> +\t{ formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },\n> > >>> };\n> > >>> \n> > >>> } /* namespace */\n> > >>> @@ -138,7 +139,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > >>> \tconst std::vector formats = cfg.formats().pixelformats();\n> > >>> \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) == formats.end()) {\n> > >>> \t\tLOG(VIMC, Debug) << \"Adjusting format to BGR888\";\n> > >>> -\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > >>> +\t\tcfg.pixelFormat = formats::BGR888;\n> > >>> \t\tstatus = Adjusted;\n> > >>> \t}\n> > >>> \n> > >>> @@ -184,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > >>> \t\t * but it isn't functional within the pipeline.\n> > >>> \t\t */\n> > >>> \t\tif (data->media_->version() < KERNEL_VERSION(5, 7, 0)) {\n> > >>> -\t\t\tif (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) {\n> > >>> +\t\t\tif (pixelformat.first != formats::BGR888) {\n> > >>> \t\t\t\tLOG(VIMC, Info)\n> > >>> \t\t\t\t\t<< \"Skipping unsupported pixel format \"\n> > >>> \t\t\t\t\t<< pixelformat.first.toString();\n> > >>> @@ -201,7 +202,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > >>> \n> > >>> \tStreamConfiguration cfg(formats);\n> > >>> \n> > >>> -\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > >>> +\tcfg.pixelFormat = formats::BGR888;\n> > >>> \tcfg.size = { 1920, 1080 };\n> > >>> \tcfg.bufferCount = 4;\n> > >>> \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89079603BF\n\tfor ;\n\tWed, 17 Jun 2020 11:32:15 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id e4so2027878ljn.4\n\tfor ;\n\tWed, 17 Jun 2020 02:32:15 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\to18sm4886536ljd.32.2020.06.17.02.32.13\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 17 Jun 2020 02:32:13 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"HMuUgfkK\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=b9Bq21J0luo17Z8xkUE2x0BmcFCAQow/Y5GiBhu+riY=;\n\tb=HMuUgfkKMybZb0f98i3hHdUK0CBMWfL2P9gjEnX6qNV0o3wccZIBr50cpQBRwRjit/\n\tL/l5Y/27rgaJFfqJ+s0WsmIMmTADBB/s6TFYTIZRLe/ewmKxCwmekvSpPQrrGKAs5R9R\n\tGuKL9LcJYktT1XyGyq5U8HDH0M5dpcqXm5GvEKUkDEGC7pq7Pz4ZKfUivRIQB4Vz6WOr\n\tBuTCnwO0LNW9ce1wFUxvUcyhpLZhlFct4YXlmgS1kWaV6MYxRvxsGmmO+GJ1WntRHC4w\n\tYxnN6FBAQlSrk7+ZWfb1jEYIpF45+D7Rw9zOrUN+W5QJuBcDVePPNUUBMIGX80nt2AnN\n\tKyBA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=b9Bq21J0luo17Z8xkUE2x0BmcFCAQow/Y5GiBhu+riY=;\n\tb=rWHAX0PFk6GPa2BRVUfa9UyOenkoJT9pMUlo2ivCKS6cJuLZY0HmEdCdowNaBuBnlG\n\tOA5O1QDtZ6U9aBEMTPbYJY4LW1KGZMUm8iuLAdiKrtvK7eXfvGp29UYD+dmpqbCXhAvn\n\tN59i4x/EdjuwzJ1HjrhtzicLLjZrQsc14GoYxI6WkXiKwitUiatIuBPej0G1MtANh6SS\n\t19ytmfWTkO6Eg/tEOUdw534CxxXGTN6/UvkQQPAh4wBF6dyG9OUtuzc3i/QjJMVQNGuJ\n\ta8CjqLFjoSjoZMENHZ5tU+h10nW0bUxuGSFfPnsCfRsqFc6VASX/Yi56kvwPm7cbuOw8\n\tTrhw==","X-Gm-Message-State":"AOAM530+CLUdQHMBgo4RZXxoKYHvYcXKt94F4daHsA4d0zF7omuWup4P\n\ts9VK8b09O1s9rQ7f7/6914rmJVSxL7k=","X-Google-Smtp-Source":"ABdhPJwQ//oRNB26dHK0ryEbv8n4wk6p1vrXycW4EaJfj3zr8XmKRLcAAx0HQjeW7OJPqVyFhhxiHg==","X-Received":"by 2002:a2e:b809:: with SMTP id u9mr3313006ljo.249.1592386334400;\n\tWed, 17 Jun 2020 02:32:14 -0700 (PDT)","Date":"Wed, 17 Jun 2020 11:32:13 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200617093213.GA2850317@oden.dyn.berto.se>","References":"<20200609232323.29628-7-laurent.pinchart@ideasonboard.com>\n\t<20200610130339.22998-1-laurent.pinchart@ideasonboard.com>\n\t<20200610144407.GH192296@oden.dyn.berto.se>\n\t<20200616024217.GD29596@pendragon.ideasonboard.com>\n\t<20200616152952.GA2565716@oden.dyn.berto.se>\n\t<20200616185506.GF913@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200616185506.GF913@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2.1 6/7] libcamera: pipeline: Replace\n\texplicit DRM FourCCs with libcamera formats","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","X-List-Received-Date":"Wed, 17 Jun 2020 09:32:15 -0000"}}]