[{"id":4402,"web_url":"https://patchwork.libcamera.org/comment/4402/","msgid":"<20200407224708.GV1716317@oden.dyn.berto.se>","date":"2020-04-07T22:47:08","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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-03-24 15:12:47 +0200, Laurent Pinchart wrote:\n> Pick the correct media bus format based on the video pixel format on the\n> capture node.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/vimc.cpp | 19 +++++++++----------\n> 1 file changed, 9 insertions(+), 10 deletions(-)\n> \n> This patch, while being correct (I think :-)), shouldn't be integrated,\n> as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the\n> viewfinder reports the following supported native formats (in that\n> order):\n> \n> - DRM_FORMAT_ABGR8888\n> - DRM_FORMAT_ARGB8888\n> - DRM_FORMAT_BGR888\n> - DRM_FORMAT_RGB888\n> \n> The first two formats are not supported by the VIMC pipeline handler,\n> the last two are. The third format, DRM_FORMAT_BGR888, is thus picked,\n> which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today.\n> \n> On Qt < 5.14.0, the third format isn't supported, so the fourth format,\n> DRM_FORMAT_RGB888, will be picked. This results in a different pipeline\n> configuration, with the debayering subdev source pad being configured\n> with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support\n> that, but in drivers/media/platform/vimc/vimc-debayer.c the\n> vimc_deb_set_fmt() function handles the source pad format with\n> \n> \t/*\n> \t * Do not change the format of the source pad,\n> \t * it is propagated from the sink\n> \t */\n> \tif (VIMC_IS_SRC(fmt->pad)) {\n> \t\tfmt->format = *sink_fmt;\n> \t\t/* TODO: Add support for other formats */\n> \t\tfmt->format.code = vdeb->src_code;\n> \t}\n> \n> This needs to be fixed on the kernel side.\n> \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index b04a9726efa5..ace58381fe92 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -6,8 +6,8 @@\n> */\n> \n> #include \n> -#include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -103,10 +103,10 @@ private:\n> \n> namespace {\n> \n> -static const std::array pixelformats{\n> -\tPixelFormat(DRM_FORMAT_RGB888),\n> -\tPixelFormat(DRM_FORMAT_BGR888),\n> -\tPixelFormat(DRM_FORMAT_BGRA8888),\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{ PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 },\n\nThis looks odd BGRA8888 mapping to ARGB8888, should that not be \nDRM_FORMAT_ABGR8888? Also should the 4th bus format listed in the commit \nmessage be added to this map?\n\n> };\n> \n> } /* namespace */\n> @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> \tStreamConfiguration &cfg = config_[0];\n> \n> \t/* Adjust the pixel format. */\n> -\tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n> -\t pixelformats.end()) {\n> +\tif (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) {\n> \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> \t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> \t\tstatus = Adjusted;\n> @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> \n> \tstd::map> formats;\n> \n> -\tfor (PixelFormat pixelformat : pixelformats) {\n> +\tfor (const auto &pixelformat : pixelformats) {\n> \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> \t\tstd::vector sizes{\n> \t\t\tSizeRange{ { 48, 48 }, { 4096, 2160 } }\n> \t\t};\n> -\t\tformats[pixelformat] = sizes;\n> +\t\tformats[pixelformat.first] = sizes;\n> \t}\n> \n> \tStreamConfiguration cfg(formats);\n> @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tsubformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24;\n> +\tsubformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second;\n> \tret = data->debayer_->setFormat(1, &subformat);\n> \tif (ret)\n> \t\treturn ret;\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-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6651D600F3\n\tfor ;\n\tWed, 8 Apr 2020 00:47:10 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id v16so5578494ljg.5\n\tfor ;\n\tTue, 07 Apr 2020 15:47:10 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tg4sm7927706ljn.105.2020.04.07.15.47.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 07 Apr 2020 15:47: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=\"ztDp7dCZ\"; \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=F1gJ+qHOVWxFgj8B0AD9R6n2Ob5lRlZv483OsDNqob8=;\n\tb=ztDp7dCZyAeNnsBCjdcHVIRBXthvPgYWC4BwN2toJvba7HqlGAPWxbCPwTxoNZuFIt\n\tmY2wLeHDSd7Ht0LkDs/Z0QJCnMRt1cxfRTnu0fh5y4xDsXAPrNHTHrvl6uu3DhfyFOGO\n\tWIqFqFps1hRBc6oB8WbTYNCuW20QHLatOqBWvu+Wv3lU4OBbvG6E5W7Fw1CtqUk1c8Q9\n\tQGMBqJ8DGC+SVQhyNtC8e6RbDDbl8pAu2uxXFev4mpoKPF7UJPPJ3jlvq0Y+jkS4FLgG\n\tIECZwuFTao411bcHD4oQNw2RVJ4k6SP4DoGLpK87zRq4uskxpC5o+i7CMoY6dgKup8+n\n\tL/zA==","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=F1gJ+qHOVWxFgj8B0AD9R6n2Ob5lRlZv483OsDNqob8=;\n\tb=Sn0IWhqxE6txau0hRWl0Xn5BM2JQUXobZbHh4JJ9/FIgD0vz2s/JNvsXaR6vJqqtPS\n\t4IksIH94BWBNzsinSLj9VLbTfg/0M+1bl+YBRqmIsTK9OxtnFqmQ1DBNRJfxgo/tQETd\n\tV99N9xMgX4X0eyJVt+17vCzB3uuGpfyu9srbzjr5YZu5Hadu5y8pTuWpDp8mikQGjEHD\n\tW8hbZoUctMgTDx6wTcKGiYbw2+cUG64ZqXg9+IRNArlaoLUkjyTBUpDCNoXN1P1E5B71\n\tcDojg8Egi6D2dyxTE5QbEaVZM8lecyLqKL9FVHiczQt33EQ5INpXt4dB1TR/p4OWUJYt\n\tffxQ==","X-Gm-Message-State":"AGi0PubqGLaRtE/dGAcZtq4Ij2Tq53QmTmUWoc84rU10hwFt5r10lY8k\n\tdyngxk2PzOC+CBcBv6e+sTN/kQ==","X-Google-Smtp-Source":"APiQypJ4jzygmWxLEWj1XSmVhE86bzvpXnGeNsO/VnuJaTajWdz5Co9+L5V5u97E+huFQfW/SN651w==","X-Received":"by 2002:a2e:81cc:: with SMTP id s12mr3098284ljg.90.1586299629584;\n\tTue, 07 Apr 2020 15:47:09 -0700 (PDT)","Date":"Wed, 8 Apr 2020 00:47:08 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200407224708.GV1716317@oden.dyn.berto.se>","References":"<20200324131247.20771-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":"<20200324131247.20771-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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, 07 Apr 2020 22:47:10 -0000"}},{"id":4423,"web_url":"https://patchwork.libcamera.org/comment/4423/","msgid":"<20200408003959.GQ4751@pendragon.ideasonboard.com>","date":"2020-04-08T00:39:59","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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, Apr 08, 2020 at 12:47:08AM +0200, Niklas Söderlund wrote:\n> On 2020-03-24 15:12:47 +0200, Laurent Pinchart wrote:\n> > Pick the correct media bus format based on the video pixel format on the\n> > capture node.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/vimc.cpp | 19 +++++++++----------\n> > 1 file changed, 9 insertions(+), 10 deletions(-)\n> > \n> > This patch, while being correct (I think :-)), shouldn't be integrated,\n> > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the\n> > viewfinder reports the following supported native formats (in that\n> > order):\n> > \n> > - DRM_FORMAT_ABGR8888\n> > - DRM_FORMAT_ARGB8888\n> > - DRM_FORMAT_BGR888\n> > - DRM_FORMAT_RGB888\n> > \n> > The first two formats are not supported by the VIMC pipeline handler,\n> > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked,\n> > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today.\n> > \n> > On Qt < 5.14.0, the third format isn't supported, so the fourth format,\n> > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline\n> > configuration, with the debayering subdev source pad being configured\n> > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support\n> > that, but in drivers/media/platform/vimc/vimc-debayer.c the\n> > vimc_deb_set_fmt() function handles the source pad format with\n> > \n> > \t/*\n> > \t * Do not change the format of the source pad,\n> > \t * it is propagated from the sink\n> > \t */\n> > \tif (VIMC_IS_SRC(fmt->pad)) {\n> > \t\tfmt->format = *sink_fmt;\n> > \t\t/* TODO: Add support for other formats */\n> > \t\tfmt->format.code = vdeb->src_code;\n> > \t}\n> > \n> > This needs to be fixed on the kernel side.\n> > \n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index b04a9726efa5..ace58381fe92 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -6,8 +6,8 @@\n> > */\n> > \n> > #include \n> > -#include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -103,10 +103,10 @@ private:\n> > \n> > namespace {\n> > \n> > -static const std::array pixelformats{\n> > -\tPixelFormat(DRM_FORMAT_RGB888),\n> > -\tPixelFormat(DRM_FORMAT_BGR888),\n> > -\tPixelFormat(DRM_FORMAT_BGRA8888),\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{ PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 },\n> \n> This looks odd BGRA8888 mapping to ARGB8888, should that not be \n> DRM_FORMAT_ABGR8888?\n\nThe vimc drivers maps MEDIA_BUS_FMT_ARGB8888_1X32 to\nV4L2_PIX_FMT_ARGB32, which corresponds to DRM_FORMAT_BGRA8888. The\nmapping may be considered awkward, but it matches the driver\nimplementation.\n\n> Also should the 4th bus format listed in the commit message be added\n> to this map?\n\nNow I'm confused. The commit message lists DRM pixel formats, not bus\nformats. Is that what you meant ? The fourth one is DRM_FORMAT_RGB888,\nwhich is listed here, but maybe you meant the missing one from the four\n? That would be DRM_FORMAT_ABGR8888, which corresponds to\nV4L2_PIX_FMT_RGBA32, which is not supported by vimc. I've thus left it\nout from this patch.\n\n> > };\n> > \n> > } /* namespace */\n> > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > \tStreamConfiguration &cfg = config_[0];\n> > \n> > \t/* Adjust the pixel format. */\n> > -\tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n> > -\t pixelformats.end()) {\n> > +\tif (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) {\n> > \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> > \t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > \t\tstatus = Adjusted;\n> > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > \n> > \tstd::map> formats;\n> > \n> > -\tfor (PixelFormat pixelformat : pixelformats) {\n> > +\tfor (const auto &pixelformat : pixelformats) {\n> > \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> > \t\tstd::vector sizes{\n> > \t\t\tSizeRange{ { 48, 48 }, { 4096, 2160 } }\n> > \t\t};\n> > -\t\tformats[pixelformat] = sizes;\n> > +\t\tformats[pixelformat.first] = sizes;\n> > \t}\n> > \n> > \tStreamConfiguration cfg(formats);\n> > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > -\tsubformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24;\n> > +\tsubformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second;\n> > \tret = data->debayer_->setFormat(1, &subformat);\n> > \tif (ret)\n> > \t\treturn ret;","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 28533600F3\n\tfor ;\n\tWed, 8 Apr 2020 02:40:11 +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 3A7A759E;\n\tWed, 8 Apr 2020 02:40:10 +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=\"R86BxJbv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1586306410;\n\tbh=AA3oemTxqgmjbhDTIGdnrvES/tKQO+F97d3ygH28zDI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R86BxJbvmhIMDV0oBv1ZkSixOm4EYYNoifjCVi0AWQ5B2PzwpDWi7QH+hmSqAjRFD\n\tFVZH2L/edxsG+9pueHBWXustDlF2Q4W0j3lIXYc2lGnwjuNeEycO2BsZzrkcLDJusb\n\ti6YOYW/1Ai98KfP19jv9XAMJrW7fynK4qiAggi0Q=","Date":"Wed, 8 Apr 2020 03:39:59 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200408003959.GQ4751@pendragon.ideasonboard.com>","References":"<20200324131247.20771-1-laurent.pinchart@ideasonboard.com>\n\t<20200407224708.GV1716317@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":"<20200407224708.GV1716317@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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, 08 Apr 2020 00:40:11 -0000"}},{"id":4424,"web_url":"https://patchwork.libcamera.org/comment/4424/","msgid":"<20200408091235.GA2340684@oden.dyn.berto.se>","date":"2020-04-08T09:12:35","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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-04-08 03:39:59 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Wed, Apr 08, 2020 at 12:47:08AM +0200, Niklas Söderlund wrote:\n> > On 2020-03-24 15:12:47 +0200, Laurent Pinchart wrote:\n> > > Pick the correct media bus format based on the video pixel format on the\n> > > capture node.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/pipeline/vimc.cpp | 19 +++++++++----------\n> > > 1 file changed, 9 insertions(+), 10 deletions(-)\n> > > \n> > > This patch, while being correct (I think :-)), shouldn't be integrated,\n> > > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the\n> > > viewfinder reports the following supported native formats (in that\n> > > order):\n> > > \n> > > - DRM_FORMAT_ABGR8888\n> > > - DRM_FORMAT_ARGB8888\n> > > - DRM_FORMAT_BGR888\n> > > - DRM_FORMAT_RGB888\n> > > \n> > > The first two formats are not supported by the VIMC pipeline handler,\n> > > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked,\n> > > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today.\n> > > \n> > > On Qt < 5.14.0, the third format isn't supported, so the fourth format,\n> > > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline\n> > > configuration, with the debayering subdev source pad being configured\n> > > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support\n> > > that, but in drivers/media/platform/vimc/vimc-debayer.c the\n> > > vimc_deb_set_fmt() function handles the source pad format with\n> > > \n> > > \t/*\n> > > \t * Do not change the format of the source pad,\n> > > \t * it is propagated from the sink\n> > > \t */\n> > > \tif (VIMC_IS_SRC(fmt->pad)) {\n> > > \t\tfmt->format = *sink_fmt;\n> > > \t\t/* TODO: Add support for other formats */\n> > > \t\tfmt->format.code = vdeb->src_code;\n> > > \t}\n> > > \n> > > This needs to be fixed on the kernel side.\n> > > \n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index b04a9726efa5..ace58381fe92 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -6,8 +6,8 @@\n> > > */\n> > > \n> > > #include \n> > > -#include \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \n> > > @@ -103,10 +103,10 @@ private:\n> > > \n> > > namespace {\n> > > \n> > > -static const std::array pixelformats{\n> > > -\tPixelFormat(DRM_FORMAT_RGB888),\n> > > -\tPixelFormat(DRM_FORMAT_BGR888),\n> > > -\tPixelFormat(DRM_FORMAT_BGRA8888),\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{ PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 },\n> > \n> > This looks odd BGRA8888 mapping to ARGB8888, should that not be \n> > DRM_FORMAT_ABGR8888?\n> \n> The vimc drivers maps MEDIA_BUS_FMT_ARGB8888_1X32 to\n> V4L2_PIX_FMT_ARGB32, which corresponds to DRM_FORMAT_BGRA8888. The\n> mapping may be considered awkward, but it matches the driver\n> implementation.\n\nI see, thanks for pointing it out.\n\n> \n> > Also should the 4th bus format listed in the commit message be added\n> > to this map?\n> \n> Now I'm confused. The commit message lists DRM pixel formats, not bus\n> formats. Is that what you meant ?\n\nYes, sorry for saying bus format instead :-)\n\n> The fourth one is DRM_FORMAT_RGB888,\n> which is listed here, but maybe you meant the missing one from the four\n> ?\n\nYes I was referring to the missing one from the list, not the 4th one.\n\n> That would be DRM_FORMAT_ABGR8888, which corresponds to\n> V4L2_PIX_FMT_RGBA32, which is not supported by vimc. I've thus left it\n> out from this patch.\n\nSorry I was confused. I assumed vimc would would support it as the \ncommit message stated 'viewfinder reports' and that was missing was \nsupport for it in the pipeline handler. Thus I asked if support for it \nshould have been added. I now see what you mean, sorry for not verifying \nthis in the vimc sources before sending my review.\n\n> \n> > > };\n> > > \n> > > } /* namespace */\n> > > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > > \tStreamConfiguration &cfg = config_[0];\n> > > \n> > > \t/* Adjust the pixel format. */\n> > > -\tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n> > > -\t pixelformats.end()) {\n> > > +\tif (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) {\n> > > \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> > > \t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > > \t\tstatus = Adjusted;\n> > > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > > \n> > > \tstd::map> formats;\n> > > \n> > > -\tfor (PixelFormat pixelformat : pixelformats) {\n> > > +\tfor (const auto &pixelformat : pixelformats) {\n> > > \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> > > \t\tstd::vector sizes{\n> > > \t\t\tSizeRange{ { 48, 48 }, { 4096, 2160 } }\n> > > \t\t};\n> > > -\t\tformats[pixelformat] = sizes;\n> > > +\t\tformats[pixelformat.first] = sizes;\n> > > \t}\n> > > \n> > > \tStreamConfiguration cfg(formats);\n> > > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > \n> > > -\tsubformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24;\n> > > +\tsubformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second;\n> > > \tret = data->debayer_->setFormat(1, &subformat);\n> > > \tif (ret)\n> > > \t\treturn ret;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F7F76277E\n\tfor ;\n\tWed, 8 Apr 2020 11:12:38 +0200 (CEST)","by mail-lf1-x144.google.com with SMTP id j17so4570767lfe.7\n\tfor ;\n\tWed, 08 Apr 2020 02:12:38 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\ty23sm13091726ljh.42.2020.04.08.02.12.36\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 08 Apr 2020 02:12:36 -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=\"qlDCB38F\"; \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=OMvSc1svb7+h8fOslET7l/wCdTg3ZXirnRHN9B4HFZk=;\n\tb=qlDCB38F57sPJXh5Yjk9Q+N1O7vHdE+aL0pIBZEbGMDmj2Ziec0iadxgOWU9XS0hcs\n\te1kKnPrUVMA5zxRzGnWT0zl25NkwlMDcubsUwPw9BmNfj51Cy13JjSbAGhSgfPBujYT1\n\tKk4zOECEjm4+P3x+cxm/YBEF1eHkOkgJGROIoLtP6yhdH00SlWVf+8LGAk5HrmVdTD0N\n\t+nOg15RK1CNOe+MKoemfzGCbnY3Ok637C7gHDVEUDxsouodfdyBh7idAvqBs5UcLArcH\n\t6PrFrzVgf1zflWOvKocKOaDYvKebDQ70YmIg8f2RTdGSMOqJYdes5TfAhbwQwztoB5NH\n\t+U3w==","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=OMvSc1svb7+h8fOslET7l/wCdTg3ZXirnRHN9B4HFZk=;\n\tb=Ln8owOFWZJOgjqYS8GYFujR7DUdawdoVJ2TRFQzzwAlsGDVO+KaQjzHlnpTM6P8E7S\n\tIVgnrJToSJPp6nto5DGMwNRXXRyHKptQzNKSkDl+JKevz1r7/H1jNkctmBxG0kAlFYtQ\n\tJfvwW4dl1U1y/d0cHdAFEMRKzrdwDB0agAMvLiT13JM6o69p0LdRO84a6ZzF1fLVG/TE\n\t+N/2Q3ow797gMDRpRWIuDKwYWZMxioG2EB1ZXiPkHHZl04KHFvV2IVb1pWCEEmhZFIHV\n\t1JLJityz5jW0fNwoGsKd9EcZ/eD3wH7PD9tWvapF4kBN8Nuox8idgynze31vcu3HWoYf\n\tS2JA==","X-Gm-Message-State":"AGi0PuZ8PF7NBB2TZyQnexy8KL46xD6VkFWz9lcsXKBln0m8AXkFliqD\n\tx9dctpwA6zGwvqU13B+gziWEaw==","X-Google-Smtp-Source":"APiQypJCFRKp8AbBHbowPu2MQSSWQ/qVBd1yJQ2MQ1AT6VPM5rVwycFZArutqW3I5peK9m3Zi+y0sA==","X-Received":"by 2002:a19:e00e:: with SMTP id\n\tx14mr4017036lfg.111.1586337157281; \n\tWed, 08 Apr 2020 02:12:37 -0700 (PDT)","Date":"Wed, 8 Apr 2020 11:12:35 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200408091235.GA2340684@oden.dyn.berto.se>","References":"<20200324131247.20771-1-laurent.pinchart@ideasonboard.com>\n\t<20200407224708.GV1716317@oden.dyn.berto.se>\n\t<20200408003959.GQ4751@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":"<20200408003959.GQ4751@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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, 08 Apr 2020 09:12:38 -0000"}},{"id":4922,"web_url":"https://patchwork.libcamera.org/comment/4922/","msgid":"","date":"2020-05-28T09:13:21","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/03/2020 13:12, Laurent Pinchart wrote:\n> Pick the correct media bus format based on the video pixel format on the\n> capture node.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/vimc.cpp | 19 +++++++++----------\n> 1 file changed, 9 insertions(+), 10 deletions(-)\n> \n> This patch, while being correct (I think :-)), shouldn't be integrated,\n> as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the\n> viewfinder reports the following supported native formats (in that\n> order):\n\n\nIn fact, VIMC camera support is currently broken anyway at the moment,\n(as per \"[RFC PATCH] libcamera: pipeline: vimc: Skip unsupported\nformats\") and I believe applying this patch doesn't actually break\nanything further.\n\nIt does however help progress development of the VIMC pipeline handler\nfor supporting extra formats when they are available, and as such I\nthink it should be merged already ;-)\n\nReviewed-by: Kieran Bingham \n\n\n\n> \n> - DRM_FORMAT_ABGR8888\n> - DRM_FORMAT_ARGB8888\n> - DRM_FORMAT_BGR888\n> - DRM_FORMAT_RGB888\n> \n> The first two formats are not supported by the VIMC pipeline handler,\n> the last two are. The third format, DRM_FORMAT_BGR888, is thus picked,\n> which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today.\n> \n> On Qt < 5.14.0, the third format isn't supported, so the fourth format,\n> DRM_FORMAT_RGB888, will be picked. This results in a different pipeline\n> configuration, with the debayering subdev source pad being configured\n> with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support\n> that, but in drivers/media/platform/vimc/vimc-debayer.c the\n> vimc_deb_set_fmt() function handles the source pad format with\n\nI wonder if the issue you describe there is a symptom of / fixed by:\n\"[PATCH] qcam: viewfinder: Use correct DRM/QImage mappings\" ...\n\n--\nKieran\n\n\n> \n> \t/*\n> \t * Do not change the format of the source pad,\n> \t * it is propagated from the sink\n> \t */\n> \tif (VIMC_IS_SRC(fmt->pad)) {\n> \t\tfmt->format = *sink_fmt;\n> \t\t/* TODO: Add support for other formats */\n> \t\tfmt->format.code = vdeb->src_code;\n> \t}\n> \n> This needs to be fixed on the kernel side.\n> \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index b04a9726efa5..ace58381fe92 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -6,8 +6,8 @@\n> */\n> \n> #include \n> -#include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -103,10 +103,10 @@ private:\n> \n> namespace {\n> \n> -static const std::array pixelformats{\n> -\tPixelFormat(DRM_FORMAT_RGB888),\n> -\tPixelFormat(DRM_FORMAT_BGR888),\n> -\tPixelFormat(DRM_FORMAT_BGRA8888),\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{ PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 },\n> };\n> \n> } /* namespace */\n> @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> \tStreamConfiguration &cfg = config_[0];\n> \n> \t/* Adjust the pixel format. */\n> -\tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n> -\t pixelformats.end()) {\n> +\tif (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) {\n> \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> \t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> \t\tstatus = Adjusted;\n> @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> \n> \tstd::map> formats;\n> \n> -\tfor (PixelFormat pixelformat : pixelformats) {\n> +\tfor (const auto &pixelformat : pixelformats) {\n> \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> \t\tstd::vector sizes{\n> \t\t\tSizeRange{ { 48, 48 }, { 4096, 2160 } }\n> \t\t};\n> -\t\tformats[pixelformat] = sizes;\n> +\t\tformats[pixelformat.first] = sizes;\n> \t}\n> \n> \tStreamConfiguration cfg(formats);\n> @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tsubformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24;\n> +\tsubformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second;\n> \tret = data->debayer_->setFormat(1, &subformat);\n> \tif (ret)\n> \t\treturn ret;\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 9E132603CF\n\tfor ;\n\tThu, 28 May 2020 11:13:25 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E2C132A3;\n\tThu, 28 May 2020 11:13:24 +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=\"JU5jnO3f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1590657205;\n\tbh=QR86MPrZ08UMu6cQDg6EuMa2gqQBHkkNJ6gMnvSOfes=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=JU5jnO3f9sfjGrkus+B5Hz3rOVUVqJFss21rxUB6gHMzZpMSvjSU0WWkcOzQhbUFn\n\trcgLwz3ijoHZEoiV7tsLG0nSG1SqY7Og1xUFv/iNc4T6ql5hmGVYxAReGjTV2QPN2R\n\t780HObmvXgKu7w69Ped0aDqRrCvBRqlApahZOoVk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200324131247.20771-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 28 May 2020 10:13:21 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200324131247.20771-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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":"Thu, 28 May 2020 09:13:25 -0000"}},{"id":5007,"web_url":"https://patchwork.libcamera.org/comment/5007/","msgid":"<20200604080258.GC5828@pendragon.ideasonboard.com>","date":"2020-06-04T08:02:58","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, May 28, 2020 at 10:13:21AM +0100, Kieran Bingham wrote:\n> On 24/03/2020 13:12, Laurent Pinchart wrote:\n> > Pick the correct media bus format based on the video pixel format on the\n> > capture node.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/vimc.cpp | 19 +++++++++----------\n> > 1 file changed, 9 insertions(+), 10 deletions(-)\n> > \n> > This patch, while being correct (I think :-)), shouldn't be integrated,\n> > as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the\n> > viewfinder reports the following supported native formats (in that\n> > order):\n> \n> In fact, VIMC camera support is currently broken anyway at the moment,\n> (as per \"[RFC PATCH] libcamera: pipeline: vimc: Skip unsupported\n> formats\") and I believe applying this patch doesn't actually break\n> anything further.\n> \n> It does however help progress development of the VIMC pipeline handler\n> for supporting extra formats when they are available, and as such I\n> think it should be merged already ;-)\n> \n> Reviewed-by: Kieran Bingham \n\nI've now merged the patch. Do you plan to submit a new version of your\nvimc pipeline handler fix ?\n\n> > - DRM_FORMAT_ABGR8888\n> > - DRM_FORMAT_ARGB8888\n> > - DRM_FORMAT_BGR888\n> > - DRM_FORMAT_RGB888\n> > \n> > The first two formats are not supported by the VIMC pipeline handler,\n> > the last two are. The third format, DRM_FORMAT_BGR888, is thus picked,\n> > which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today.\n> > \n> > On Qt < 5.14.0, the third format isn't supported, so the fourth format,\n> > DRM_FORMAT_RGB888, will be picked. This results in a different pipeline\n> > configuration, with the debayering subdev source pad being configured\n> > with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support\n> > that, but in drivers/media/platform/vimc/vimc-debayer.c the\n> > vimc_deb_set_fmt() function handles the source pad format with\n> \n> I wonder if the issue you describe there is a symptom of / fixed by:\n> \"[PATCH] qcam: viewfinder: Use correct DRM/QImage mappings\" ...\n> \n> > \n> > \t/*\n> > \t * Do not change the format of the source pad,\n> > \t * it is propagated from the sink\n> > \t */\n> > \tif (VIMC_IS_SRC(fmt->pad)) {\n> > \t\tfmt->format = *sink_fmt;\n> > \t\t/* TODO: Add support for other formats */\n> > \t\tfmt->format.code = vdeb->src_code;\n> > \t}\n> > \n> > This needs to be fixed on the kernel side.\n> > \n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index b04a9726efa5..ace58381fe92 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -6,8 +6,8 @@\n> > */\n> > \n> > #include \n> > -#include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -103,10 +103,10 @@ private:\n> > \n> > namespace {\n> > \n> > -static const std::array pixelformats{\n> > -\tPixelFormat(DRM_FORMAT_RGB888),\n> > -\tPixelFormat(DRM_FORMAT_BGR888),\n> > -\tPixelFormat(DRM_FORMAT_BGRA8888),\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{ PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 },\n> > };\n> > \n> > } /* namespace */\n> > @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > \tStreamConfiguration &cfg = config_[0];\n> > \n> > \t/* Adjust the pixel format. */\n> > -\tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n> > -\t pixelformats.end()) {\n> > +\tif (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) {\n> > \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> > \t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);\n> > \t\tstatus = Adjusted;\n> > @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > \n> > \tstd::map> formats;\n> > \n> > -\tfor (PixelFormat pixelformat : pixelformats) {\n> > +\tfor (const auto &pixelformat : pixelformats) {\n> > \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> > \t\tstd::vector sizes{\n> > \t\t\tSizeRange{ { 48, 48 }, { 4096, 2160 } }\n> > \t\t};\n> > -\t\tformats[pixelformat] = sizes;\n> > +\t\tformats[pixelformat.first] = sizes;\n> > \t}\n> > \n> > \tStreamConfiguration cfg(formats);\n> > @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > -\tsubformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24;\n> > +\tsubformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second;\n> > \tret = data->debayer_->setFormat(1, &subformat);\n> > \tif (ret)\n> > \t\treturn ret;","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 79625603C8\n\tfor ;\n\tThu, 4 Jun 2020 10:03:15 +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 DF52529B;\n\tThu, 4 Jun 2020 10:03:14 +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=\"B+atUO8b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591257795;\n\tbh=bhvmRqKkklGXgMb6IQC6caTYlKtISmaGws6+2KZbbUk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B+atUO8bGpCJgweN3qpN2XAfoiBTcszlHjrt6B5J3Gii3pJeYoAUWM3wEjdAeOUfQ\n\tKqW5pFKlIdXzvaZjx087AxYuUjKiUNJlOyR8LySc/2LptExp2DE9cyCQMButmAruPh\n\t+byLA9UeiTUaCA/QoLeInoTCrwTjox68tY9t1KrA=","Date":"Thu, 4 Jun 2020 11:02:58 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200604080258.GC5828@pendragon.ideasonboard.com>","References":"<20200324131247.20771-1-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use\n\tappropriate media bus format","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":"Thu, 04 Jun 2020 08:03:15 -0000"}}]