[{"id":12066,"web_url":"https://patchwork.libcamera.org/comment/12066/","msgid":"<20200820101225.2kh3caz3u26frbhr@uno.localdomain>","date":"2020-08-20T10:12:25","subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 13, 2020 at 02:52:45AM +0200, Niklas Söderlund wrote:\n> Extend the format validation to work with both man and self path. The\n> heuristics honors that the first stream in the configuration have the\n> highest priority while still examining both streams for a best match.\n>\n> This change extends the formats supported by the Cameras backed by this\n> pipeline to also include RGB888 and RGB656, that is only available on\n> the self path.\n>\n> It is not possible to capture from the self path as the self stream is\n> not yet exposed to applications.\n>\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 220 ++++++++++++++++++-----\n>  1 file changed, 171 insertions(+), 49 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 3761b7ef7a9386e3..f106b105a47bb4f7 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -9,6 +9,7 @@\n>  #include <array>\n>  #include <iomanip>\n>  #include <memory>\n> +#include <numeric>\n>  #include <queue>\n>\n>  #include <linux/media-bus-format.h>\n> @@ -50,6 +51,20 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{\n>  \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n>  };\n>\n> +static const Size RKISP1_RSZ_SP_SRC_MIN = { 32, 16 };\n> +static const Size RKISP1_RSZ_SP_SRC_MAX = { 1920, 1920 };\n> +static const std::array<PixelFormat, 9> RKISP1_RSZ_SP_FORMATS{\n\nsame comments as in previous patches.\n\nConsidering how you have to transport the template in the validatePath\nfunction, I would use vectors here for the formats enumeration.\n\n> +\tformats::YUYV,\n> +\tformats::YVYU,\n> +\tformats::VYUY,\n> +\tformats::NV16,\n> +\tformats::NV61,\n> +\tformats::NV21,\n> +\tformats::NV12,\n> +\tformats::BGR888,\n> +\tformats::RGB565,\n> +};\n> +\n>  class PipelineHandlerRkISP1;\n>  class RkISP1ActionQueueBuffers;\n>  class RkISP1CameraData;\n> @@ -179,13 +194,23 @@ public:\n>  private:\n>  \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n>\n> +\tbool fitAnyPath(const StreamConfiguration &cfg);\n> +\n> +\ttemplate<std::size_t N>\n> +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> +\t\t\t\t\t\t const std::array<PixelFormat, N> &formats,\n\nA vector would be much nicer here.\n\n> +\t\t\t\t\t\t const Size &min, const Size &max,\n> +\t\t\t\t\t\t V4L2VideoDevice *video);\n> +\tCameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);\n> +\tCameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);\n> +\n>  \t/*\n>  \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n>  \t * corresponding Camera instance is valid. In order to borrow a\n>  \t * reference to the camera data, store a new reference to the camera.\n>  \t */\n>  \tstd::shared_ptr<Camera> camera_;\n> -\tconst RkISP1CameraData *data_;\n> +\tRkISP1CameraData *data_;\n>\n>  \tV4L2SubdeviceFormat sensorFormat_;\n>  };\n> @@ -495,6 +520,76 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n>  \tdata_ = data;\n>  }\n>\n> +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)\n> +{\n> +\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(),\n> +\t\t      RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==\n> +\t    RKISP1_RSZ_MP_FORMATS.end())\n> +\t\treturn false;\n> +\n> +\tif (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)\n> +\t\treturn false;\n> +\n> +\tif (std::find(RKISP1_RSZ_SP_FORMATS.begin(),\n> +\t\t      RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==\n> +\t    RKISP1_RSZ_SP_FORMATS.end())\n> +\t\treturn false;\n> +\n> +\tif (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +template<std::size_t N>\n> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> +\tStreamConfiguration *cfg, const std::array<PixelFormat, N> &formats,\n> +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\n> +{\n> +\tconst StreamConfiguration reqCfg = *cfg;\n> +\tStatus status = Valid;\n> +\n> +\tif (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==\n> +\t    formats.end())\n> +\t\tcfg->pixelFormat = formats::NV12;\n> +\n> +\tcfg->size.boundTo(max);\n> +\tcfg->size.expandTo(min);\n> +\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> +\n> +\tV4L2DeviceFormat format = {};\n> +\tformat.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);\n> +\tformat.size = cfg->size;\n> +\n> +\tint ret = video->tryFormat(&format);\n> +\tif (ret)\n> +\t\treturn Invalid;\n\nDo we need to go to the video device here ? We know the format/sizes\nare supported already...\n\n> +\n> +\tcfg->stride = format.planes[0].bpl;\n> +\tcfg->frameSize = format.planes[0].size;\n> +\n> +\tif (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {\n> +\t\tLOG(RkISP1, Debug)\n> +\t\t\t<< \"Adjusting format from \" << reqCfg.toString()\n> +\t\t\t<< \" to \" << cfg->toString();\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)\n> +{\n> +\treturn validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,\n> +\t\t\t    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);\n> +}\n> +\n> +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)\n> +{\n> +\treturn validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,\n> +\t\t\t    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);\n> +}\n> +\n>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  {\n>  \tconst CameraSensor *sensor = data_->sensor_;\n> @@ -504,22 +599,86 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \t\treturn Invalid;\n>\n>  \t/* Cap the number of entries to the available streams. */\n> -\tif (config_.size() > 1) {\n> -\t\tconfig_.resize(1);\n> +\tif (config_.size() > 2) {\n> +\t\tconfig_.resize(2);\n>  \t\tstatus = Adjusted;\n>  \t}\n>\n> -\tStreamConfiguration &cfg = config_[0];\n> -\n> -\t/* Adjust the pixel format. */\n> -\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),\n> -\t\t      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {\n> -\t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> -\t\tcfg.pixelFormat = formats::NV12,\n> -\t\tstatus = Adjusted;\n> +\t/*\n> +\t * If there are more then one stream in the configuration figure out the\n> +\t * order to evaluate them streams. The first stream have the highest\n> +\t * priority but if both main path and self path can satisfy it evaluate\n> +\t * second stream first.\n> +\t */\n> +\tstd::vector<unsigned int> order(config_.size());\n> +\tstd::iota(order.begin(), order.end(), 0);\n> +\tif (config_.size() == 2 && fitAnyPath(config_[0]))\n> +\t\tstd::reverse(order.begin(), order.end());\n\nNice, but -why- ? What are the criteria you use to decide which\nconfigu gets to which stream ? In example, I see the self path being\nlimited in sizes compared to the main, shouldn't this be taken into\naccount ? Same for the supported formats. How to you establish the\nhere reported 'order' ?\n\n> +\n> +\tbool mainPathAvailable = true;\n> +\tbool selfPathAvailable = true;\n> +\tfor (unsigned int index : order) {\n> +\t\tStreamConfiguration &cfg = config_[index];\n> +\n> +\t\t/* Try to match stream without adjusting configuration. */\n> +\t\tif (mainPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateMainPath(&tryCfg) == Valid) {\n> +\t\t\t\tmainPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match main\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (selfPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateSelfPath(&tryCfg) == Valid) {\n> +\t\t\t\tselfPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match self\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/* Try to match stream allowing adjusting configuration. */\n> +\t\tif (mainPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateMainPath(&tryCfg) == Adjusted) {\n> +\t\t\t\tmainPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match main\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (selfPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateSelfPath(&tryCfg) == Adjusted) {\n\nCan't you get the status from here instead of repeating the check for\neach stream 2 times ? You would only need to return Invalid if you\ndetect it, otherwise assign the value of validate*Path() to status.\n\nThe only other reason I see here to keep 4 if switches is to printout\nthe result, but that could indeed be done depending on the returned\nstatus.\n\n> +\t\t\t\tselfPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/* All paths rejected configuraiton. */\n> +\t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> +\t\t\t\t   << cfg.toString();\n> +\t\treturn Invalid;\n>  \t}\n>\n>  \t/* Select the sensor format. */\n> +\tSize maxSize;\n> +\tfor (const StreamConfiguration &cfg : config_)\n> +\t\tmaxSize = std::max(maxSize, cfg.size);\n> +\n>  \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SGBRG12_1X12,\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SGRBG12_1X12,\n> @@ -532,47 +691,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SGBRG8_1X8,\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SGRBG8_1X8,\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SRGGB8_1X8 },\n> -\t\t\t\t\t  cfg.size);\n> +\t\t\t\t\t  maxSize);\n>  \tif (sensorFormat_.size.isNull())\n>  \t\tsensorFormat_.size = sensor->resolution();\n>\n> -\t/*\n> -\t * Provide a suitable default that matches the sensor aspect\n> -\t * ratio and clamp the size to the hardware bounds.\n> -\t *\n> -\t * \\todo: Check the hardware alignment constraints.\n> -\t */\n> -\tconst Size size = cfg.size;\n> -\n> -\tif (cfg.size.isNull()) {\n> -\t\tcfg.size.width = 1280;\n> -\t\tcfg.size.height = 1280 * sensorFormat_.size.height\n> -\t\t\t\t/ sensorFormat_.size.width;\n> -\t}\n> -\n> -\tcfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);\n> -\tcfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);\n> -\n> -\tif (cfg.size != size) {\n> -\t\tLOG(RkISP1, Debug)\n> -\t\t\t<< \"Adjusting size from \" << size.toString()\n> -\t\t\t<< \" to \" << cfg.size.toString();\n> -\t\tstatus = Adjusted;\n> -\t}\n> -\n> -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> -\n> -\tV4L2DeviceFormat format = {};\n> -\tformat.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> -\tformat.size = cfg.size;\n> -\n> -\tint ret = data_->mainPathVideo_->tryFormat(&format);\n> -\tif (ret)\n> -\t\treturn Invalid;\n> -\n> -\tcfg.stride = format.planes[0].bpl;\n> -\tcfg.frameSize = format.planes[0].size;\n> -\n>  \treturn status;\n>  }\n>\n> --\n> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 964C7BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 20 Aug 2020 10:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF89761F0F;\n\tThu, 20 Aug 2020 12:08:44 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16C0360381\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 20 Aug 2020 12:08:43 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 19ADA4000D;\n\tThu, 20 Aug 2020 10:08:41 +0000 (UTC)"],"X-Originating-IP":"82.52.18.94","Date":"Thu, 20 Aug 2020 12:12:25 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200820101225.2kh3caz3u26frbhr@uno.localdomain>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-13-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200813005246.3265807-13-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12498,"web_url":"https://patchwork.libcamera.org/comment/12498/","msgid":"<20200914114820.GL1127199@oden.dyn.berto.se>","date":"2020-09-14T11:48:20","subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-08-20 12:12:25 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 13, 2020 at 02:52:45AM +0200, Niklas Söderlund wrote:\n> > Extend the format validation to work with both man and self path. The\n> > heuristics honors that the first stream in the configuration have the\n> > highest priority while still examining both streams for a best match.\n> >\n> > This change extends the formats supported by the Cameras backed by this\n> > pipeline to also include RGB888 and RGB656, that is only available on\n> > the self path.\n> >\n> > It is not possible to capture from the self path as the self stream is\n> > not yet exposed to applications.\n> >\n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 220 ++++++++++++++++++-----\n> >  1 file changed, 171 insertions(+), 49 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 3761b7ef7a9386e3..f106b105a47bb4f7 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -9,6 +9,7 @@\n> >  #include <array>\n> >  #include <iomanip>\n> >  #include <memory>\n> > +#include <numeric>\n> >  #include <queue>\n> >\n> >  #include <linux/media-bus-format.h>\n> > @@ -50,6 +51,20 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{\n> >  \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> >  };\n> >\n> > +static const Size RKISP1_RSZ_SP_SRC_MIN = { 32, 16 };\n> > +static const Size RKISP1_RSZ_SP_SRC_MAX = { 1920, 1920 };\n> > +static const std::array<PixelFormat, 9> RKISP1_RSZ_SP_FORMATS{\n> \n> same comments as in previous patches.\n> \n> Considering how you have to transport the template in the validatePath\n> function, I would use vectors here for the formats enumeration.\n\nI wrestled with this while developing it I wanted to use vector but \nthought it would be frowned upon ;-) I will switch to a vector in v2, \nthis will however prevent it from being a constexpr.\n\n> \n> > +\tformats::YUYV,\n> > +\tformats::YVYU,\n> > +\tformats::VYUY,\n> > +\tformats::NV16,\n> > +\tformats::NV61,\n> > +\tformats::NV21,\n> > +\tformats::NV12,\n> > +\tformats::BGR888,\n> > +\tformats::RGB565,\n> > +};\n> > +\n> >  class PipelineHandlerRkISP1;\n> >  class RkISP1ActionQueueBuffers;\n> >  class RkISP1CameraData;\n> > @@ -179,13 +194,23 @@ public:\n> >  private:\n> >  \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> >\n> > +\tbool fitAnyPath(const StreamConfiguration &cfg);\n> > +\n> > +\ttemplate<std::size_t N>\n> > +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> > +\t\t\t\t\t\t const std::array<PixelFormat, N> &formats,\n> \n> A vector would be much nicer here.\n> \n> > +\t\t\t\t\t\t const Size &min, const Size &max,\n> > +\t\t\t\t\t\t V4L2VideoDevice *video);\n> > +\tCameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);\n> > +\tCameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);\n> > +\n> >  \t/*\n> >  \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n> >  \t * corresponding Camera instance is valid. In order to borrow a\n> >  \t * reference to the camera data, store a new reference to the camera.\n> >  \t */\n> >  \tstd::shared_ptr<Camera> camera_;\n> > -\tconst RkISP1CameraData *data_;\n> > +\tRkISP1CameraData *data_;\n> >\n> >  \tV4L2SubdeviceFormat sensorFormat_;\n> >  };\n> > @@ -495,6 +520,76 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> >  \tdata_ = data;\n> >  }\n> >\n> > +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)\n> > +{\n> > +\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(),\n> > +\t\t      RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==\n> > +\t    RKISP1_RSZ_MP_FORMATS.end())\n> > +\t\treturn false;\n> > +\n> > +\tif (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)\n> > +\t\treturn false;\n> > +\n> > +\tif (std::find(RKISP1_RSZ_SP_FORMATS.begin(),\n> > +\t\t      RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==\n> > +\t    RKISP1_RSZ_SP_FORMATS.end())\n> > +\t\treturn false;\n> > +\n> > +\tif (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +template<std::size_t N>\n> > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> > +\tStreamConfiguration *cfg, const std::array<PixelFormat, N> &formats,\n> > +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\n> > +{\n> > +\tconst StreamConfiguration reqCfg = *cfg;\n> > +\tStatus status = Valid;\n> > +\n> > +\tif (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==\n> > +\t    formats.end())\n> > +\t\tcfg->pixelFormat = formats::NV12;\n> > +\n> > +\tcfg->size.boundTo(max);\n> > +\tcfg->size.expandTo(min);\n> > +\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> > +\n> > +\tV4L2DeviceFormat format = {};\n> > +\tformat.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);\n> > +\tformat.size = cfg->size;\n> > +\n> > +\tint ret = video->tryFormat(&format);\n> > +\tif (ret)\n> > +\t\treturn Invalid;\n> \n> Do we need to go to the video device here ? We know the format/sizes\n> are supported already...\n\nI do this to retrieve the true size as there might be aliment issues and \nsuch which we can retrieve from the kernel instead of mirroring it in \nthe pipeline.\n\n> \n> > +\n> > +\tcfg->stride = format.planes[0].bpl;\n> > +\tcfg->frameSize = format.planes[0].size;\n> > +\n> > +\tif (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {\n> > +\t\tLOG(RkISP1, Debug)\n> > +\t\t\t<< \"Adjusting format from \" << reqCfg.toString()\n> > +\t\t\t<< \" to \" << cfg->toString();\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> > +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)\n> > +{\n> > +\treturn validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,\n> > +\t\t\t    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);\n> > +}\n> > +\n> > +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)\n> > +{\n> > +\treturn validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,\n> > +\t\t\t    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);\n> > +}\n> > +\n> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  {\n> >  \tconst CameraSensor *sensor = data_->sensor_;\n> > @@ -504,22 +599,86 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  \t\treturn Invalid;\n> >\n> >  \t/* Cap the number of entries to the available streams. */\n> > -\tif (config_.size() > 1) {\n> > -\t\tconfig_.resize(1);\n> > +\tif (config_.size() > 2) {\n> > +\t\tconfig_.resize(2);\n> >  \t\tstatus = Adjusted;\n> >  \t}\n> >\n> > -\tStreamConfiguration &cfg = config_[0];\n> > -\n> > -\t/* Adjust the pixel format. */\n> > -\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),\n> > -\t\t      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {\n> > -\t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > -\t\tcfg.pixelFormat = formats::NV12,\n> > -\t\tstatus = Adjusted;\n> > +\t/*\n> > +\t * If there are more then one stream in the configuration figure out the\n> > +\t * order to evaluate them streams. The first stream have the highest\n> > +\t * priority but if both main path and self path can satisfy it evaluate\n> > +\t * second stream first.\n> > +\t */\n> > +\tstd::vector<unsigned int> order(config_.size());\n> > +\tstd::iota(order.begin(), order.end(), 0);\n> > +\tif (config_.size() == 2 && fitAnyPath(config_[0]))\n> > +\t\tstd::reverse(order.begin(), order.end());\n> \n> Nice, but -why- ? What are the criteria you use to decide which\n> configu gets to which stream ? In example, I see the self path being\n> limited in sizes compared to the main, shouldn't this be taken into\n> account ? Same for the supported formats. How to you establish the\n> here reported 'order' ?\n\nIt is in the fitAnyPath() check is it not? The design is based on that \nthe first stream i the configuration requested have the highest priority \nto be satisfied. If both main and self path can satisfy the highest \npriority we reverse the priority to allow for the second stream to have \nthe highest chance to also meets it requested configuration (or get as \nclose as possible to it).\n\n> \n> > +\n> > +\tbool mainPathAvailable = true;\n> > +\tbool selfPathAvailable = true;\n> > +\tfor (unsigned int index : order) {\n> > +\t\tStreamConfiguration &cfg = config_[index];\n> > +\n> > +\t\t/* Try to match stream without adjusting configuration. */\n> > +\t\tif (mainPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateMainPath(&tryCfg) == Valid) {\n> > +\t\t\t\tmainPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match main\";\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (selfPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateSelfPath(&tryCfg) == Valid) {\n> > +\t\t\t\tselfPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match self\";\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\t/* Try to match stream allowing adjusting configuration. */\n> > +\t\tif (mainPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateMainPath(&tryCfg) == Adjusted) {\n> > +\t\t\t\tmainPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> > +\t\t\t\tstatus = Adjusted;\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match main\";\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (selfPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateSelfPath(&tryCfg) == Adjusted) {\n> \n> Can't you get the status from here instead of repeating the check for\n> each stream 2 times ? You would only need to return Invalid if you\n> detect it, otherwise assign the value of validate*Path() to status.\n> \n> The only other reason I see here to keep 4 if switches is to printout\n> the result, but that could indeed be done depending on the returned\n> status.\n\nNo I can't, this needs to be a two step process. One pass to try and get \nan exact match one one where adjustments are allowed. If we merge the \ntwo the first stream will always be mapped to the main path (either as a \ndirect match or adjusted) and the second stream to the self path.\n\nBy doing it as a two step pass we allow for an exact match on the self \npath where it would have been accepted as an adjusted match on the main \npath.\n\n> \n> > +\t\t\t\tselfPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> > +\t\t\t\tstatus = Adjusted;\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\t/* All paths rejected configuraiton. */\n> > +\t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> > +\t\t\t\t   << cfg.toString();\n> > +\t\treturn Invalid;\n> >  \t}\n> >\n> >  \t/* Select the sensor format. */\n> > +\tSize maxSize;\n> > +\tfor (const StreamConfiguration &cfg : config_)\n> > +\t\tmaxSize = std::max(maxSize, cfg.size);\n> > +\n> >  \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> >  \t\t\t\t\t    MEDIA_BUS_FMT_SGBRG12_1X12,\n> >  \t\t\t\t\t    MEDIA_BUS_FMT_SGRBG12_1X12,\n> > @@ -532,47 +691,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  \t\t\t\t\t    MEDIA_BUS_FMT_SGBRG8_1X8,\n> >  \t\t\t\t\t    MEDIA_BUS_FMT_SGRBG8_1X8,\n> >  \t\t\t\t\t    MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > -\t\t\t\t\t  cfg.size);\n> > +\t\t\t\t\t  maxSize);\n> >  \tif (sensorFormat_.size.isNull())\n> >  \t\tsensorFormat_.size = sensor->resolution();\n> >\n> > -\t/*\n> > -\t * Provide a suitable default that matches the sensor aspect\n> > -\t * ratio and clamp the size to the hardware bounds.\n> > -\t *\n> > -\t * \\todo: Check the hardware alignment constraints.\n> > -\t */\n> > -\tconst Size size = cfg.size;\n> > -\n> > -\tif (cfg.size.isNull()) {\n> > -\t\tcfg.size.width = 1280;\n> > -\t\tcfg.size.height = 1280 * sensorFormat_.size.height\n> > -\t\t\t\t/ sensorFormat_.size.width;\n> > -\t}\n> > -\n> > -\tcfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);\n> > -\tcfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);\n> > -\n> > -\tif (cfg.size != size) {\n> > -\t\tLOG(RkISP1, Debug)\n> > -\t\t\t<< \"Adjusting size from \" << size.toString()\n> > -\t\t\t<< \" to \" << cfg.size.toString();\n> > -\t\tstatus = Adjusted;\n> > -\t}\n> > -\n> > -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > -\n> > -\tV4L2DeviceFormat format = {};\n> > -\tformat.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> > -\tformat.size = cfg.size;\n> > -\n> > -\tint ret = data_->mainPathVideo_->tryFormat(&format);\n> > -\tif (ret)\n> > -\t\treturn Invalid;\n> > -\n> > -\tcfg.stride = format.planes[0].bpl;\n> > -\tcfg.frameSize = format.planes[0].size;\n> > -\n> >  \treturn status;\n> >  }\n> >\n> > --\n> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 645E5BF01C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 14 Sep 2020 11:48:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D67F262D99;\n\tMon, 14 Sep 2020 13:48:23 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7344E62B90\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Sep 2020 13:48:22 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id c2so18314588ljj.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Sep 2020 04:48:22 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ti17sm4042356lja.45.2020.09.14.04.48.20\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Sep 2020 04:48:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"ZV85uhQb\"; dkim-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=Tl8j++yF5a0/XPWL0CLImRs8QdKnNhfQTXkWSxaa/4Y=;\n\tb=ZV85uhQb0FJgzE3hG/vEzQsGPV81jocTQPT1UHnhOmpLwiRk2RlLHYkVjhFITWXfTj\n\t7k6qSi5AX2x77ozo4cWoOtOwECsaoBHXt+pXLJ+yzJqf0Kc2zSzNUZrYi9uZHoshbk2x\n\tJeYEE4wJ9VziseZafVTmNem9TNj+OQg14PCj6+blXRVYRWnHm95ZLCHt43NRWjhS+FNq\n\to2X0aoBGa2ODK4M7Ov4vSokVdjOmWx+HmxmaK5i9iG35H7J3H2BrWthuleUx3Au9uwOR\n\tm19KPRj3/Mc169GwuQ5iihiJWsSJNq0E9ifS5SgYxarfwN4HT3XgYnk9SdaPywAo0fXW\n\tZNSQ==","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=Tl8j++yF5a0/XPWL0CLImRs8QdKnNhfQTXkWSxaa/4Y=;\n\tb=FeVntPpsWNmweCV8DonJb/JgBRYywcy1SqS/DpNRyVAueDTAQpGlAhBesBeLGR61ET\n\t8wVVonvhX3LnoyLWUlirrluYeNSp9UrKlSMhaFjFw17j1k76WAPdu+MicLcImrpstkPZ\n\tpa7PYPVEn7HuLrTvMziykN+s1HOU2MDVP4oHAb7gE9X+wcD7IGdjKbUfOG6TxoKFpmjs\n\trX8+J7XV8LcNFR0Yr7tFA8OEIMvnHz6aqzlHDcMmi0KTjdO6WbgRhXW271TiVFkYqIZi\n\teiqKY+gmmLf/J7na7Otia6QLAZtT/8YIVjkQAgvGPZz2SfPBD33Jf0s2Oegbi05bj7hs\n\tP7Qg==","X-Gm-Message-State":"AOAM533iSK7bkEl8F0/51i0YikaK7Nbt8XmMcDgn1IDMg3QRAdRuQ/41\n\tznBcbEqA9fUZteZr+0xV3YtbzQ==","X-Google-Smtp-Source":"ABdhPJw48GqdL1QElHrzVrbqaJaqgkA9Qh8EM0Iz7um3eiaIeGlslWPa6x2YND01/KqAbtm7VyjKBg==","X-Received":"by 2002:a05:651c:134a:: with SMTP id\n\tj10mr5316193ljb.337.1600084101730; \n\tMon, 14 Sep 2020 04:48:21 -0700 (PDT)","Date":"Mon, 14 Sep 2020 13:48:20 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200914114820.GL1127199@oden.dyn.berto.se>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-13-niklas.soderlund@ragnatech.se>\n\t<20200820101225.2kh3caz3u26frbhr@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200820101225.2kh3caz3u26frbhr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12504,"web_url":"https://patchwork.libcamera.org/comment/12504/","msgid":"<20200915001521.GO15543@pendragon.ideasonboard.com>","date":"2020-09-15T00:15:21","subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Sep 14, 2020 at 01:48:20PM +0200, Niklas Söderlund wrote:\n> On 2020-08-20 12:12:25 +0200, Jacopo Mondi wrote:\n> > On Thu, Aug 13, 2020 at 02:52:45AM +0200, Niklas Söderlund wrote:\n> > > Extend the format validation to work with both man and self path. The\n> > > heuristics honors that the first stream in the configuration have the\n> > > highest priority while still examining both streams for a best match.\n> > >\n> > > This change extends the formats supported by the Cameras backed by this\n> > > pipeline to also include RGB888 and RGB656, that is only available on\n> > > the self path.\n> > >\n> > > It is not possible to capture from the self path as the self stream is\n> > > not yet exposed to applications.\n> > >\n> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > ---\n> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 220 ++++++++++++++++++-----\n> > >  1 file changed, 171 insertions(+), 49 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 3761b7ef7a9386e3..f106b105a47bb4f7 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -9,6 +9,7 @@\n> > >  #include <array>\n> > >  #include <iomanip>\n> > >  #include <memory>\n> > > +#include <numeric>\n> > >  #include <queue>\n> > >\n> > >  #include <linux/media-bus-format.h>\n> > > @@ -50,6 +51,20 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{\n> > >  \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > >  };\n> > >\n> > > +static const Size RKISP1_RSZ_SP_SRC_MIN = { 32, 16 };\n> > > +static const Size RKISP1_RSZ_SP_SRC_MAX = { 1920, 1920 };\n> > > +static const std::array<PixelFormat, 9> RKISP1_RSZ_SP_FORMATS{\n> > \n> > same comments as in previous patches.\n> > \n> > Considering how you have to transport the template in the validatePath\n> > function, I would use vectors here for the formats enumeration.\n> \n> I wrestled with this while developing it I wanted to use vector but \n> thought it would be frowned upon ;-) I will switch to a vector in v2, \n> this will however prevent it from being a constexpr.\n\nstd::array will be more efficient though. You can simplify\nvalidatePath() by passing a Span<PixelFormat>, so it won't need to be a\ntemplate function.\n\n> > > +\tformats::YUYV,\n> > > +\tformats::YVYU,\n> > > +\tformats::VYUY,\n> > > +\tformats::NV16,\n> > > +\tformats::NV61,\n> > > +\tformats::NV21,\n> > > +\tformats::NV12,\n> > > +\tformats::BGR888,\n> > > +\tformats::RGB565,\n> > > +};\n> > > +\n> > >  class PipelineHandlerRkISP1;\n> > >  class RkISP1ActionQueueBuffers;\n> > >  class RkISP1CameraData;\n> > > @@ -179,13 +194,23 @@ public:\n> > >  private:\n> > >  \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> > >\n> > > +\tbool fitAnyPath(const StreamConfiguration &cfg);\n> > > +\n> > > +\ttemplate<std::size_t N>\n> > > +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> > > +\t\t\t\t\t\t const std::array<PixelFormat, N> &formats,\n> > \n> > A vector would be much nicer here.\n> > \n> > > +\t\t\t\t\t\t const Size &min, const Size &max,\n> > > +\t\t\t\t\t\t V4L2VideoDevice *video);\n> > > +\tCameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);\n> > > +\tCameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);\n> > > +\n> > >  \t/*\n> > >  \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n> > >  \t * corresponding Camera instance is valid. In order to borrow a\n> > >  \t * reference to the camera data, store a new reference to the camera.\n> > >  \t */\n> > >  \tstd::shared_ptr<Camera> camera_;\n> > > -\tconst RkISP1CameraData *data_;\n> > > +\tRkISP1CameraData *data_;\n> > >\n> > >  \tV4L2SubdeviceFormat sensorFormat_;\n> > >  };\n> > > @@ -495,6 +520,76 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > >  \tdata_ = data;\n> > >  }\n> > >\n> > > +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)\n> > > +{\n> > > +\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(),\n> > > +\t\t      RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==\n> > > +\t    RKISP1_RSZ_MP_FORMATS.end())\n> > > +\t\treturn false;\n> > > +\n> > > +\tif (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)\n> > > +\t\treturn false;\n> > > +\n> > > +\tif (std::find(RKISP1_RSZ_SP_FORMATS.begin(),\n> > > +\t\t      RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==\n> > > +\t    RKISP1_RSZ_SP_FORMATS.end())\n> > > +\t\treturn false;\n> > > +\n> > > +\tif (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +template<std::size_t N>\n> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> > > +\tStreamConfiguration *cfg, const std::array<PixelFormat, N> &formats,\n> > > +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\n> > > +{\n> > > +\tconst StreamConfiguration reqCfg = *cfg;\n> > > +\tStatus status = Valid;\n> > > +\n> > > +\tif (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==\n> > > +\t    formats.end())\n> > > +\t\tcfg->pixelFormat = formats::NV12;\n> > > +\n> > > +\tcfg->size.boundTo(max);\n> > > +\tcfg->size.expandTo(min);\n> > > +\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> > > +\n> > > +\tV4L2DeviceFormat format = {};\n> > > +\tformat.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);\n> > > +\tformat.size = cfg->size;\n> > > +\n> > > +\tint ret = video->tryFormat(&format);\n> > > +\tif (ret)\n> > > +\t\treturn Invalid;\n> > \n> > Do we need to go to the video device here ? We know the format/sizes\n> > are supported already...\n> \n> I do this to retrieve the true size as there might be aliment issues and \n> such which we can retrieve from the kernel instead of mirroring it in \n> the pipeline.\n> \n> > \n> > > +\n> > > +\tcfg->stride = format.planes[0].bpl;\n> > > +\tcfg->frameSize = format.planes[0].size;\n> > > +\n> > > +\tif (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {\n> > > +\t\tLOG(RkISP1, Debug)\n> > > +\t\t\t<< \"Adjusting format from \" << reqCfg.toString()\n> > > +\t\t\t<< \" to \" << cfg->toString();\n> > > +\t\tstatus = Adjusted;\n> > > +\t}\n> > > +\n> > > +\treturn status;\n> > > +}\n> > > +\n> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)\n> > > +{\n> > > +\treturn validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,\n> > > +\t\t\t    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);\n> > > +}\n> > > +\n> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)\n> > > +{\n> > > +\treturn validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,\n> > > +\t\t\t    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);\n> > > +}\n> > > +\n> > >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >  {\n> > >  \tconst CameraSensor *sensor = data_->sensor_;\n> > > @@ -504,22 +599,86 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >  \t\treturn Invalid;\n> > >\n> > >  \t/* Cap the number of entries to the available streams. */\n> > > -\tif (config_.size() > 1) {\n> > > -\t\tconfig_.resize(1);\n> > > +\tif (config_.size() > 2) {\n> > > +\t\tconfig_.resize(2);\n> > >  \t\tstatus = Adjusted;\n> > >  \t}\n> > >\n> > > -\tStreamConfiguration &cfg = config_[0];\n> > > -\n> > > -\t/* Adjust the pixel format. */\n> > > -\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),\n> > > -\t\t      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {\n> > > -\t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > > -\t\tcfg.pixelFormat = formats::NV12,\n> > > -\t\tstatus = Adjusted;\n> > > +\t/*\n> > > +\t * If there are more then one stream in the configuration figure out the\n> > > +\t * order to evaluate them streams. The first stream have the highest\n> > > +\t * priority but if both main path and self path can satisfy it evaluate\n> > > +\t * second stream first.\n> > > +\t */\n> > > +\tstd::vector<unsigned int> order(config_.size());\n> > > +\tstd::iota(order.begin(), order.end(), 0);\n> > > +\tif (config_.size() == 2 && fitAnyPath(config_[0]))\n> > > +\t\tstd::reverse(order.begin(), order.end());\n> > \n> > Nice, but -why- ? What are the criteria you use to decide which\n> > configu gets to which stream ? In example, I see the self path being\n> > limited in sizes compared to the main, shouldn't this be taken into\n> > account ? Same for the supported formats. How to you establish the\n> > here reported 'order' ?\n> \n> It is in the fitAnyPath() check is it not? The design is based on that \n> the first stream i the configuration requested have the highest priority \n> to be satisfied. If both main and self path can satisfy the highest \n> priority we reverse the priority to allow for the second stream to have \n> the highest chance to also meets it requested configuration (or get as \n> close as possible to it).\n> \n> > > +\n> > > +\tbool mainPathAvailable = true;\n> > > +\tbool selfPathAvailable = true;\n> > > +\tfor (unsigned int index : order) {\n> > > +\t\tStreamConfiguration &cfg = config_[index];\n> > > +\n> > > +\t\t/* Try to match stream without adjusting configuration. */\n> > > +\t\tif (mainPathAvailable) {\n> > > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > > +\t\t\tif (validateMainPath(&tryCfg) == Valid) {\n> > > +\t\t\t\tmainPathAvailable = false;\n> > > +\t\t\t\tcfg = tryCfg;\n> > > +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> > > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match main\";\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\tif (selfPathAvailable) {\n> > > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > > +\t\t\tif (validateSelfPath(&tryCfg) == Valid) {\n> > > +\t\t\t\tselfPathAvailable = false;\n> > > +\t\t\t\tcfg = tryCfg;\n> > > +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> > > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match self\";\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\t/* Try to match stream allowing adjusting configuration. */\n> > > +\t\tif (mainPathAvailable) {\n> > > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > > +\t\t\tif (validateMainPath(&tryCfg) == Adjusted) {\n> > > +\t\t\t\tmainPathAvailable = false;\n> > > +\t\t\t\tcfg = tryCfg;\n> > > +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> > > +\t\t\t\tstatus = Adjusted;\n> > > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match main\";\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\tif (selfPathAvailable) {\n> > > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > > +\t\t\tif (validateSelfPath(&tryCfg) == Adjusted) {\n> > \n> > Can't you get the status from here instead of repeating the check for\n> > each stream 2 times ? You would only need to return Invalid if you\n> > detect it, otherwise assign the value of validate*Path() to status.\n> > \n> > The only other reason I see here to keep 4 if switches is to printout\n> > the result, but that could indeed be done depending on the returned\n> > status.\n> \n> No I can't, this needs to be a two step process. One pass to try and get \n> an exact match one one where adjustments are allowed. If we merge the \n> two the first stream will always be mapped to the main path (either as a \n> direct match or adjusted) and the second stream to the self path.\n> \n> By doing it as a two step pass we allow for an exact match on the self \n> path where it would have been accepted as an adjusted match on the main \n> path.\n> \n> > > +\t\t\t\tselfPathAvailable = false;\n> > > +\t\t\t\tcfg = tryCfg;\n> > > +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> > > +\t\t\t\tstatus = Adjusted;\n> > > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\t/* All paths rejected configuraiton. */\n> > > +\t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> > > +\t\t\t\t   << cfg.toString();\n> > > +\t\treturn Invalid;\n> > >  \t}\n> > >\n> > >  \t/* Select the sensor format. */\n> > > +\tSize maxSize;\n> > > +\tfor (const StreamConfiguration &cfg : config_)\n> > > +\t\tmaxSize = std::max(maxSize, cfg.size);\n> > > +\n> > >  \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> > >  \t\t\t\t\t    MEDIA_BUS_FMT_SGBRG12_1X12,\n> > >  \t\t\t\t\t    MEDIA_BUS_FMT_SGRBG12_1X12,\n> > > @@ -532,47 +691,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >  \t\t\t\t\t    MEDIA_BUS_FMT_SGBRG8_1X8,\n> > >  \t\t\t\t\t    MEDIA_BUS_FMT_SGRBG8_1X8,\n> > >  \t\t\t\t\t    MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > > -\t\t\t\t\t  cfg.size);\n> > > +\t\t\t\t\t  maxSize);\n> > >  \tif (sensorFormat_.size.isNull())\n> > >  \t\tsensorFormat_.size = sensor->resolution();\n> > >\n> > > -\t/*\n> > > -\t * Provide a suitable default that matches the sensor aspect\n> > > -\t * ratio and clamp the size to the hardware bounds.\n> > > -\t *\n> > > -\t * \\todo: Check the hardware alignment constraints.\n> > > -\t */\n> > > -\tconst Size size = cfg.size;\n> > > -\n> > > -\tif (cfg.size.isNull()) {\n> > > -\t\tcfg.size.width = 1280;\n> > > -\t\tcfg.size.height = 1280 * sensorFormat_.size.height\n> > > -\t\t\t\t/ sensorFormat_.size.width;\n> > > -\t}\n> > > -\n> > > -\tcfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);\n> > > -\tcfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);\n> > > -\n> > > -\tif (cfg.size != size) {\n> > > -\t\tLOG(RkISP1, Debug)\n> > > -\t\t\t<< \"Adjusting size from \" << size.toString()\n> > > -\t\t\t<< \" to \" << cfg.size.toString();\n> > > -\t\tstatus = Adjusted;\n> > > -\t}\n> > > -\n> > > -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > > -\n> > > -\tV4L2DeviceFormat format = {};\n> > > -\tformat.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> > > -\tformat.size = cfg.size;\n> > > -\n> > > -\tint ret = data_->mainPathVideo_->tryFormat(&format);\n> > > -\tif (ret)\n> > > -\t\treturn Invalid;\n> > > -\n> > > -\tcfg.stride = format.planes[0].bpl;\n> > > -\tcfg.frameSize = format.planes[0].size;\n> > > -\n> > >  \treturn status;\n> > >  }\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CB7E4C3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 15 Sep 2020 00:15:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D8DE62DAB;\n\tTue, 15 Sep 2020 02:15:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1908D62901\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Sep 2020 02:15:50 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80127275;\n\tTue, 15 Sep 2020 02:15:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uglfGf4C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600128949;\n\tbh=tELlwdvLcQogS19og/fhXb2wGhRNG7g21zMdh1W+Nc0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uglfGf4CE9WcfszdJx1rQTcwkxP5JDSKE99+0zXVRxuBTDk0Wb22C4q/8qY1bh6qM\n\t76obA06IqFTNJqCRj74j0ZruxYGywxv6WqQyErE2HvhiWewzBLy46w+O2er9l7zF3V\n\tqK7cTuW3quaum/VRekCqrzGDpnzmFRxFC0OBhHes=","Date":"Tue, 15 Sep 2020 03:15:21 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200915001521.GO15543@pendragon.ideasonboard.com>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-13-niklas.soderlund@ragnatech.se>\n\t<20200820101225.2kh3caz3u26frbhr@uno.localdomain>\n\t<20200914114820.GL1127199@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200914114820.GL1127199@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]