[{"id":12831,"web_url":"https://patchwork.libcamera.org/comment/12831/","msgid":"<20200928223928.GT23539@pendragon.ideasonboard.com>","date":"2020-09-28T22:39:28","subject":"Re: [libcamera-devel] [PATCH v3 17/22] libcamera: pipeline: rkisp1:\n\tMove path configuration generation and validation to RkISP1Path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Sep 25, 2020 at 03:42:02AM +0200, Niklas Söderlund wrote:\n> Move the path configuration generation and validation to RkISP1Path.\n> This is done to increase code reuse and to encapsulate the main and self\n> path differences inside the RkISP1Path class. There is no functional\n> change.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 115 ++-----------------\n>  src/libcamera/pipeline/rkisp1/rkisp1path.cpp |  88 +++++++++++++-\n>  src/libcamera/pipeline/rkisp1/rkisp1path.h   |  18 ++-\n>  3 files changed, 112 insertions(+), 109 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 2403eec4691b5ff6..114aee3e180afb77 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -20,7 +20,6 @@\n>  #include <libcamera/formats.h>\n>  #include <libcamera/ipa/rkisp1.h>\n>  #include <libcamera/request.h>\n> -#include <libcamera/span.h>\n>  #include <libcamera/stream.h>\n>  \n>  #include \"libcamera/internal/camera_sensor.h\"\n> @@ -40,34 +39,6 @@ namespace libcamera {\n>  \n>  LOG_DEFINE_CATEGORY(RkISP1)\n>  \n> -namespace {\n> -constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };\n> -constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };\n> -constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{\n> -\tformats::YUYV,\n> -\tformats::YVYU,\n> -\tformats::VYUY,\n> -\tformats::NV16,\n> -\tformats::NV61,\n> -\tformats::NV21,\n> -\tformats::NV12,\n> -\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> -};\n> -\n> -constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> -constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };\n> -constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{\n> -\tformats::YUYV,\n> -\tformats::YVYU,\n> -\tformats::VYUY,\n> -\tformats::NV16,\n> -\tformats::NV61,\n> -\tformats::NV21,\n> -\tformats::NV12,\n> -\t/* \\todo Add support for BGR888 and RGB565 */\n> -};\n> -} /* namespace */\n> -\n>  class PipelineHandlerRkISP1;\n>  class RkISP1ActionQueueBuffers;\n>  class RkISP1CameraData;\n> @@ -194,14 +165,6 @@ public:\n>  \tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n>  \n>  private:\n> -\tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> -\n> -\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> -\t\t\t\t\t\t const Span<const PixelFormat> &formats,\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>  \tbool fitsAllPaths(const StreamConfiguration &cfg);\n>  \n>  \t/*\n> @@ -514,64 +477,16 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n>  \tdata_ = data;\n>  }\n>  \n> -CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> -\tStreamConfiguration *cfg, const Span<const PixelFormat> &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> -\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_->mainPath_->video_);\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_->selfPath_->video_);\n> -}\n> -\n>  bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n>  {\n>  \tStreamConfiguration config;\n>  \n>  \tconfig = cfg;\n> -\tif (validateMainPath(&config) != Valid)\n> +\tif (data_->mainPath_->validate(&config) != Valid)\n>  \t\treturn false;\n>  \n>  \tconfig = cfg;\n> -\tif (validateSelfPath(&config) != Valid)\n> +\tif (data_->selfPath_->validate(&config) != Valid)\n>  \t\treturn false;\n>  \n>  \treturn true;\n> @@ -611,7 +526,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\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\tif (data_->mainPath_->validate(&tryCfg) == Valid) {\n>  \t\t\t\tmainPathAvailable = false;\n>  \t\t\t\tcfg = tryCfg;\n>  \t\t\t\tcfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));\n> @@ -622,7 +537,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \n>  \t\tif (selfPathAvailable) {\n>  \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (validateSelfPath(&tryCfg) == Valid) {\n> +\t\t\tif (data_->selfPath_->validate(&tryCfg) == Valid) {\n>  \t\t\t\tselfPathAvailable = false;\n>  \t\t\t\tcfg = tryCfg;\n>  \t\t\t\tcfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));\n> @@ -634,7 +549,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\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\tif (data_->mainPath_->validate(&tryCfg) == Adjusted) {\n>  \t\t\t\tmainPathAvailable = false;\n>  \t\t\t\tcfg = tryCfg;\n>  \t\t\t\tcfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));\n> @@ -646,7 +561,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \n>  \t\tif (selfPathAvailable) {\n>  \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (validateSelfPath(&tryCfg) == Adjusted) {\n> +\t\t\tif (data_->selfPath_->validate(&tryCfg) == Adjusted) {\n>  \t\t\t\tselfPathAvailable = false;\n>  \t\t\t\tcfg = tryCfg;\n>  \t\t\t\tcfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));\n> @@ -734,25 +649,17 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n>  \t\t\treturn nullptr;\n>  \t\t}\n>  \n> -\t\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> +\t\tStreamConfiguration cfg;\n>  \t\tif (useMainPath) {\n> +\t\t\tcfg = data->mainPath_->generateConfiguration(\n> +\t\t\t\tdata->sensor_->resolution());\n>  \t\t\tmainPathAvailable = false;\n> -\t\t\tfor (const PixelFormat &format : RKISP1_RSZ_MP_FORMATS)\n> -\t\t\t\tstreamFormats[format] =\n> -\t\t\t\t{ { RKISP1_RSZ_MP_SRC_MIN, data->sensor_->resolution() } };\n>  \t\t} else {\n> +\t\t\tcfg = data->selfPath_->generateConfiguration(\n> +\t\t\t\tdata->sensor_->resolution());\n>  \t\t\tselfPathAvailable = false;\n> -\t\t\tfor (const PixelFormat &format : RKISP1_RSZ_SP_FORMATS)\n> -\t\t\t\tstreamFormats[format] =\n> -\t\t\t\t{ { RKISP1_RSZ_SP_SRC_MIN, data->sensor_->resolution() } };\n>  \t\t}\n>  \n> -\t\tStreamFormats formats(streamFormats);\n> -\t\tStreamConfiguration cfg(formats);\n> -\t\tcfg.pixelFormat = formats::NV12;\n> -\t\tcfg.size = data->sensor_->resolution();\n> -\t\tcfg.bufferCount = 4;\n> -\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> index 758580934817ed6a..0be4d20bb1cd2094 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> @@ -7,6 +7,7 @@\n>  \n>  #include \"rkisp1path.h\"\n>  \n> +#include <libcamera/formats.h>\n>  #include <libcamera/stream.h>\n>  \n>  #include \"libcamera/internal/media_device.h\"\n> @@ -17,8 +18,11 @@ namespace libcamera {\n>  \n>  LOG_DECLARE_CATEGORY(RkISP1)\n>  \n> -RkISP1Path::RkISP1Path(const char *name)\n> -\t: video_(nullptr), name_(name), resizer_(nullptr)\n> +RkISP1Path::RkISP1Path(const char *name, const std::vector<PixelFormat> &formats,\n> +\t\t       const Size &minResolution, const Size &maxResolution)\n> +\t: video_(nullptr), name_(name), formats_(formats),\n> +\t  minResolution_(minResolution), maxResolution_(maxResolution),\n> +\t  resizer_(nullptr)\n>  {\n>  }\n>  \n> @@ -44,6 +48,58 @@ bool RkISP1Path::init(MediaDevice *media)\n>  \treturn true;\n>  }\n>  \n> +StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)\n> +{\n> +\tSize maxResolution = resolution;\n> +\tmaxResolution.boundTo(maxResolution_);\n> +\n> +\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> +\tfor (const PixelFormat &format : formats_)\n> +\t\tstreamFormats[format] = { { minResolution_, maxResolution } };\n> +\n> +\tStreamFormats formats(streamFormats);\n> +\tStreamConfiguration cfg(formats);\n> +\tcfg.pixelFormat = formats::NV12;\n> +\tcfg.size = maxResolution;\n> +\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> +\n> +\treturn cfg;\n> +}\n> +\n> +CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)\n> +{\n> +\tconst StreamConfiguration reqCfg = *cfg;\n> +\tCameraConfiguration::Status status = CameraConfiguration::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(maxResolution_);\n> +\tcfg->size.expandTo(minResolution_);\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 CameraConfiguration::Invalid;\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 = CameraConfiguration::Adjusted;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n>  int RkISP1Path::configure(const StreamConfiguration &config,\n>  \t\t\t  const V4L2SubdeviceFormat &inputFormat)\n>  {\n> @@ -94,12 +150,36 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n>  }\n>  \n>  RkISP1MainPath::RkISP1MainPath()\n> -\t: RkISP1Path(\"main\")\n> +\t: RkISP1Path(\"main\",\n> +\t\t     {\n> +\t\t\t     formats::YUYV,\n> +\t\t\t     formats::YVYU,\n> +\t\t\t     formats::VYUY,\n> +\t\t\t     formats::NV16,\n> +\t\t\t     formats::NV61,\n> +\t\t\t     formats::NV21,\n> +\t\t\t     formats::NV12,\n> +\t\t\t     /* \\todo Add support for 8-bit greyscale to DRM formats */\n> +\t\t     },\n> +\t\t     { 32, 16 },\n> +\t\t     { 4416, 3312 })\n>  {\n>  }\n>  \n>  RkISP1SelfPath::RkISP1SelfPath()\n> -\t: RkISP1Path(\"self\")\n> +\t: RkISP1Path(\"self\",\n> +\t\t     {\n> +\t\t\t     formats::YUYV,\n> +\t\t\t     formats::YVYU,\n> +\t\t\t     formats::VYUY,\n> +\t\t\t     formats::NV16,\n> +\t\t\t     formats::NV61,\n> +\t\t\t     formats::NV21,\n> +\t\t\t     formats::NV12,\n> +\t\t\t     /* \\todo Add support for BGR888 and RGB565 */\n> +\t\t     },\n> +\t\t     { 32, 16 },\n> +\t\t     { 1920, 1920 })\n>  {\n>  }\n>  \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> index 6eb01529d2fddb1c..5b2917c746ee1d95 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> @@ -7,9 +7,15 @@\n>  #ifndef __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n>  #define __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n>  \n> +#include <vector>\n> +\n> +#include <libcamera/camera.h>\n> +\n>  namespace libcamera {\n>  \n>  class MediaDevice;\n> +class PixelFormat;\n> +class Size;\n\nYou need to include the corresponding headers to avoid depending on\nindirect includes, as those classes are embedded in RkISP1Path.\n\n>  class V4L2Subdevice;\n>  class V4L2VideoDevice;\n>  struct StreamConfiguration;\n> @@ -18,11 +24,15 @@ struct V4L2SubdeviceFormat;\n>  class RkISP1Path\n>  {\n>  public:\n> -\tRkISP1Path(const char *name);\n> +\tRkISP1Path(const char *name, const std::vector<PixelFormat> &formats,\n> +\t\t   const Size &minResolution, const Size &maxResolution);\n>  \t~RkISP1Path();\n>  \n>  \tbool init(MediaDevice *media);\n>  \n> +\tStreamConfiguration generateConfiguration(const Size &resolution);\n> +\tCameraConfiguration::Status validate(StreamConfiguration *cfg);\n> +\n>  \tint configure(const StreamConfiguration &config,\n>  \t\t      const V4L2SubdeviceFormat &inputFormat);\n>  \n> @@ -30,8 +40,14 @@ public:\n>  \tV4L2VideoDevice *video_;\n>  \n>  private:\n> +\tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> +\n>  \tconst char *name_;\n>  \n> +\tconst std::vector<PixelFormat> formats_;\n\nCould we keep the formats array as constexpr globals and store a span\nhere (and of course move them to rkisp1path.cpp) ? I'm tempted to do the\nsame for the min/max sizes, as that would look less like magic numbers.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\tconst Size minResolution_;\n> +\tconst Size maxResolution_;\n> +\n>  \tV4L2Subdevice *resizer_;\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 DA880C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Sep 2020 22:40:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5BE4160BF7;\n\tTue, 29 Sep 2020 00:40:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AE1560394\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Sep 2020 00:40:03 +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 D6A3FA58;\n\tTue, 29 Sep 2020 00:40:02 +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=\"a3lSQUxT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601332803;\n\tbh=8LKcelIQ/TLBU2MvQQPPj1Kqd6MQXPUe2wadNf4ZboM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a3lSQUxTOAUTjdoJ5JhmW2T6DTso5fm2/gz3i0rGtYx4zAAqI8GVX4dSdxuFCJ22F\n\tBmFnGDEUgIcsu2iuxZcbps9mMahiYyyaFbj9/4yN3uCAOeC1rh8JfcoLVs7WnnlTnh\n\tmjgbVAuhJIYx3mPOE7YOiEy2hqtCljy2TepqOMfU=","Date":"Tue, 29 Sep 2020 01:39:28 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200928223928.GT23539@pendragon.ideasonboard.com>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-18-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-18-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 17/22] libcamera: pipeline: rkisp1:\n\tMove path configuration generation and validation to RkISP1Path","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>"}}]