[{"id":12778,"web_url":"https://patchwork.libcamera.org/comment/12778/","msgid":"<20200925151203.dpo4gcnutun7gf4c@uno.localdomain>","date":"2020-09-25T15:12:03","subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n  wow a long subject!\n\nOn Fri, Sep 25, 2020 at 03:42:00AM +0200, Niklas Söderlund wrote:\n> The self and main paths are very similar and the introduction of support\n> for two paths simulating sly (paths) have made it clear their handling\n\nIs it me not knowing what \"simulating sly (paths)\" means ?\n\n> could be abstracted in a separate class.\n>\n> This is the first step to create such an class by breaking out the\n> initialization and storage of the video and subdevices. There is no\n> functional change in this patch.\n>\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/pipeline/rkisp1/meson.build    |   1 +\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 104 ++++++++-----------\n>  src/libcamera/pipeline/rkisp1/rkisp1path.cpp |  53 ++++++++++\n>  src/libcamera/pipeline/rkisp1/rkisp1path.h   |  46 ++++++++\n>  4 files changed, 144 insertions(+), 60 deletions(-)\n>  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n>  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.h\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/meson.build b/src/libcamera/pipeline/rkisp1/meson.build\n> index 1ab3964a6db190f0..eddf795e54575956 100644\n> --- a/src/libcamera/pipeline/rkisp1/meson.build\n> +++ b/src/libcamera/pipeline/rkisp1/meson.build\n> @@ -2,5 +2,6 @@\n>\n>  libcamera_sources += files([\n>      'rkisp1.cpp',\n> +    'rkisp1path.cpp',\n>      'timeline.cpp',\n>  ])\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 27a3c44da3805c5f..e738a7eb19264d79 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -33,6 +33,7 @@\n>  #include \"libcamera/internal/v4l2_subdevice.h\"\n>  #include \"libcamera/internal/v4l2_videodevice.h\"\n>\n> +#include \"rkisp1path.h\"\n>  #include \"timeline.h\"\n>\n>  namespace libcamera {\n> @@ -147,12 +148,11 @@ public:\n>  class RkISP1CameraData : public CameraData\n>  {\n>  public:\n> -\tRkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *mainPathVideo,\n> -\t\t\t V4L2VideoDevice *selfPathVideo)\n> +\tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> +\t\t\t RkISP1SelfPath *selfPath)\n>  \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> -\t\t  frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n> -\t\t  selfPathVideo_(selfPathVideo), mainPathActive_(false),\n> -\t\t  selfPathActive_(false)\n> +\t\t  frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath),\n> +\t\t  mainPathActive_(false), selfPathActive_(false)\n>  \t{\n>  \t}\n>\n> @@ -171,8 +171,8 @@ public:\n>  \tRkISP1Frames frameInfo_;\n>  \tRkISP1Timeline timeline_;\n>\n> -\tV4L2VideoDevice *mainPathVideo_;\n> -\tV4L2VideoDevice *selfPathVideo_;\n> +\tRkISP1MainPath *mainPath_;\n> +\tRkISP1SelfPath *selfPath_;\n>\n>  \tbool mainPathActive_;\n>  \tbool selfPathActive_;\n> @@ -259,13 +259,12 @@ private:\n>\n>  \tMediaDevice *media_;\n>  \tV4L2Subdevice *isp_;\n> -\tV4L2Subdevice *mainPathResizer_;\n> -\tV4L2Subdevice *selfPathResizer_;\n> -\tV4L2VideoDevice *mainPathVideo_;\n> -\tV4L2VideoDevice *selfPathVideo_;\n>  \tV4L2VideoDevice *param_;\n>  \tV4L2VideoDevice *stat_;\n>\n> +\tRkISP1MainPath mainPath_;\n> +\tRkISP1SelfPath selfPath_;\n> +\n>  \tstd::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;\n>  \tstd::vector<std::unique_ptr<FrameBuffer>> statBuffers_;\n>  \tstd::queue<FrameBuffer *> availableParamBuffers_;\n> @@ -441,10 +440,10 @@ protected:\n>  \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n>\n>  \t\tif (info->mainPathBuffer)\n> -\t\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> +\t\t\tpipe_->mainPath_.video_->queueBuffer(info->mainPathBuffer);\n>\n>  \t\tif (info->selfPathBuffer)\n> -\t\t\tpipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);\n> +\t\t\tpipe_->selfPath_.video_->queueBuffer(info->selfPathBuffer);\n>  \t}\n>\n>  private:\n> @@ -554,13 +553,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\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> +\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_->selfPathVideo_);\n> +\t\t\t    RKISP1_RSZ_SP_SRC_MAX, data_->selfPath_->video_);\n>  }\n>\n>  bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> @@ -688,9 +687,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  }\n>\n>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> -\t: PipelineHandler(manager), isp_(nullptr), mainPathResizer_(nullptr),\n> -\t  selfPathResizer_(nullptr), mainPathVideo_(nullptr),\n> -\t  selfPathVideo_(nullptr), param_(nullptr), stat_(nullptr)\n> +\t: PipelineHandler(manager), isp_(nullptr), param_(nullptr),\n> +\t  stat_(nullptr)\n>  {\n>  }\n>\n> @@ -698,10 +696,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n>  {\n>  \tdelete param_;\n>  \tdelete stat_;\n> -\tdelete mainPathVideo_;\n> -\tdelete selfPathVideo_;\n> -\tdelete mainPathResizer_;\n> -\tdelete selfPathResizer_;\n>  \tdelete isp_;\n>  }\n>\n> @@ -821,12 +815,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \t\tV4L2VideoDevice *video;\n>\n>  \t\tif (cfg.stream() == &data->mainPathStream_) {\n> -\t\t\tresizer = mainPathResizer_;\n> -\t\t\tvideo = mainPathVideo_;\n> +\t\t\tresizer = mainPath_.resizer_;\n> +\t\t\tvideo = mainPath_.video_;\n>  \t\t\tdata->mainPathActive_ = true;\n>  \t\t} else {\n> -\t\t\tresizer = selfPathResizer_;\n> -\t\t\tvideo = selfPathVideo_;\n> +\t\t\tresizer = selfPath_.resizer_;\n> +\t\t\tvideo = selfPath_.video_;\n>  \t\t\tdata->selfPathActive_ = true;\n>  \t\t}\n>\n> @@ -834,7 +828,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \t\tif (ret < 0)\n>  \t\t\treturn ret;\n>\n> -\t\tconst char *name = resizer == mainPathResizer_ ? \"main\" : \"self\";\n> +\t\tconst char *name = resizer == mainPath_.resizer_ ? \"main\" : \"self\";\n>\n>  \t\tLOG(RkISP1, Debug)\n>  \t\t\t<< \"Configured \" << name << \" resizer input pad with \"\n> @@ -894,9 +888,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n>  \tunsigned int count = stream->configuration().bufferCount;\n>\n>  \tif (stream == &data->mainPathStream_)\n> -\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n> +\t\treturn mainPath_.video_->exportBuffers(count, buffers);\n>  \telse if (stream == &data->selfPathStream_)\n> -\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n> +\t\treturn selfPath_.video_->exportBuffers(count, buffers);\n>\n>  \treturn -EINVAL;\n>  }\n> @@ -913,14 +907,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>  \t});\n>\n>  \tif (data->mainPathActive_) {\n> -\t\tret = mainPathVideo_->importBuffers(\n> +\t\tret = mainPath_.video_->importBuffers(\n>  \t\t\tdata->mainPathStream_.configuration().bufferCount);\n>  \t\tif (ret < 0)\n>  \t\t\tgoto error;\n>  \t}\n>\n>  \tif (data->selfPathActive_) {\n> -\t\tret = selfPathVideo_->importBuffers(\n> +\t\tret = selfPath_.video_->importBuffers(\n>  \t\t\tdata->selfPathStream_.configuration().bufferCount);\n>  \t\tif (ret < 0)\n>  \t\t\tgoto error;\n> @@ -955,8 +949,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>  error:\n>  \tparamBuffers_.clear();\n>  \tstatBuffers_.clear();\n> -\tmainPathVideo_->releaseBuffers();\n> -\tselfPathVideo_->releaseBuffers();\n> +\tmainPath_.video_->releaseBuffers();\n> +\tselfPath_.video_->releaseBuffers();\n>\n>  \treturn ret;\n>  }\n> @@ -987,10 +981,10 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>  \tif (stat_->releaseBuffers())\n>  \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n>\n> -\tif (mainPathVideo_->releaseBuffers())\n> +\tif (mainPath_.video_->releaseBuffers())\n>  \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n>\n> -\tif (selfPathVideo_->releaseBuffers())\n> +\tif (selfPath_.video_->releaseBuffers())\n>  \t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n>\n>  \treturn 0;\n> @@ -1038,7 +1032,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \tstd::map<unsigned int, IPAStream> streamConfig;\n>\n>  \tif (data->mainPathActive_) {\n> -\t\tret = mainPathVideo_->streamOn();\n> +\t\tret = mainPath_.video_->streamOn();\n>  \t\tif (ret) {\n>  \t\t\tparam_->streamOff();\n>  \t\t\tstat_->streamOff();\n> @@ -1057,10 +1051,10 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \t}\n>\n>  \tif (data->selfPathActive_) {\n> -\t\tret = selfPathVideo_->streamOn();\n> +\t\tret = selfPath_.video_->streamOn();\n>  \t\tif (ret) {\n>  \t\t\tif (data->mainPathActive_)\n> -\t\t\t\tmainPathVideo_->streamOff();\n> +\t\t\t\tmainPath_.video_->streamOff();\n>\n>  \t\t\tparam_->streamOff();\n>  \t\t\tstat_->streamOff();\n> @@ -1106,7 +1100,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>  \tint ret;\n>\n>  \tif (data->selfPathActive_) {\n> -\t\tret = selfPathVideo_->streamOff();\n> +\t\tret = selfPath_.video_->streamOff();\n>  \t\tif (ret)\n>  \t\t\tLOG(RkISP1, Warning)\n>  \t\t\t\t<< \"Failed to stop self path for \"\n> @@ -1114,7 +1108,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>  \t}\n>\n>  \tif (data->mainPathActive_) {\n> -\t\tret = mainPathVideo_->streamOff();\n> +\t\tret = mainPath_.video_->streamOff();\n>  \t\tif (ret)\n>  \t\t\tLOG(RkISP1, Warning)\n>  \t\t\t\t<< \"Failed to stop main path for \"\n> @@ -1226,8 +1220,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \tint ret;\n>\n>  \tstd::unique_ptr<RkISP1CameraData> data =\n> -\t\tstd::make_unique<RkISP1CameraData>(this, mainPathVideo_,\n> -\t\t\t\t\t\t   selfPathVideo_);\n> +\t\tstd::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);\n>\n>  \tControlInfoMap::Map ctrls;\n>  \tctrls.emplace(std::piecewise_construct,\n> @@ -1281,23 +1274,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \tif (isp_->open() < 0)\n>  \t\treturn false;\n>\n> -\tmainPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_mainpath\");\n> -\tif (mainPathResizer_->open() < 0)\n> -\t\treturn false;\n> -\n> -\tselfPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_selfpath\");\n> -\tif (selfPathResizer_->open() < 0)\n> -\t\treturn false;\n> -\n>  \t/* Locate and open the capture video node. */\n> -\tmainPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_mainpath\");\n> -\tif (mainPathVideo_->open() < 0)\n> -\t\treturn false;\n> -\n> -\tselfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_selfpath\");\n> -\tif (selfPathVideo_->open() < 0)\n> -\t\treturn false;\n> -\n>  \tstat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_stats\");\n>  \tif (stat_->open() < 0)\n>  \t\treturn false;\n> @@ -1306,8 +1283,15 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \tif (param_->open() < 0)\n>  \t\treturn false;\n>\n> -\tmainPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> -\tselfPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\t/* Locate and open the ISP main and self paths. */\n> +\tif (!mainPath_.init(media_))\n> +\t\treturn false;\n> +\n> +\tif (!selfPath_.init(media_))\n> +\t\treturn false;\n> +\n> +\tmainPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\tselfPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n>  \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n>  \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> new file mode 100644\n> index 0000000000000000..51a75df86baaaa7b\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> @@ -0,0 +1,53 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * rkisp1path.cpp - Rockchip ISP1 path helper\n> + */\n> +\n> +#include \"rkisp1path.h\"\n> +\n> +#include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/v4l2_subdevice.h\"\n> +#include \"libcamera/internal/v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +RkISP1Path::RkISP1Path(const char *name)\n> +\t: resizer_(nullptr), video_(nullptr), name_(name)\n> +{\n> +}\n> +\n> +RkISP1Path::~RkISP1Path()\n> +{\n> +\tdelete video_;\n> +\tdelete resizer_;\n> +}\n> +\n> +bool RkISP1Path::init(MediaDevice *media)\n> +{\n> +\tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> +\tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> +\n> +\tresizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> +\tif (resizer_->open() < 0)\n> +\t\treturn false;\n> +\n> +\tvideo_ = V4L2VideoDevice::fromEntityName(media, video);\n> +\tif (video_->open() < 0)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +RkISP1MainPath::RkISP1MainPath()\n> +\t: RkISP1Path(\"main\")\n> +{\n> +}\n> +\n> +RkISP1SelfPath::RkISP1SelfPath()\n> +\t: RkISP1Path(\"self\")\n> +{\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> new file mode 100644\n> index 0000000000000000..d3172e228a3f67bf\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> @@ -0,0 +1,46 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * rkisp1path.h - Rockchip ISP1 path helper\n> + */\n> +#ifndef __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> +#define __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> +\n> +namespace libcamera {\n> +\n> +class MediaDevice;\n> +class V4L2Subdevice;\n> +class V4L2VideoDevice;\n> +\n> +class RkISP1Path\n> +{\n> +public:\n> +\tRkISP1Path(const char *name);\n> +\t~RkISP1Path();\n> +\n> +\tbool init(MediaDevice *media);\n> +\n> +\t/* \\todo Make resizer and video private. */\n\nWhat is blocking this ?\n\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n> +\tV4L2Subdevice *resizer_;\n> +\tV4L2VideoDevice *video_;\n> +\n> +private:\n> +\tconst char *name_;\n> +};\n> +\n> +class RkISP1MainPath : public RkISP1Path\n> +{\n> +public:\n> +\tRkISP1MainPath();\n> +};\n> +\n> +class RkISP1SelfPath : public RkISP1Path\n> +{\n> +public:\n> +\tRkISP1SelfPath();\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PIPELINE_RKISP1_PATH_H__ */\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 562EBC3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 25 Sep 2020 15:08:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32CC4631C4;\n\tFri, 25 Sep 2020 17:08:12 +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 DA48463046\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Sep 2020 17:08:10 +0200 (CEST)","from uno.localdomain (host-87-18-63-10.retail.telecomitalia.it\n\t[87.18.63.10]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B4FCF40015;\n\tFri, 25 Sep 2020 15:08:09 +0000 (UTC)"],"X-Originating-IP":"87.18.63.10","Date":"Fri, 25 Sep 2020 17:12:03 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200925151203.dpo4gcnutun7gf4c@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-16-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-16-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","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":12786,"web_url":"https://patchwork.libcamera.org/comment/12786/","msgid":"<20200925165319.GH1757254@oden.dyn.berto.se>","date":"2020-09-25T16:53:19","subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2020-09-25 17:12:03 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n>   wow a long subject!\n> \n> On Fri, Sep 25, 2020 at 03:42:00AM +0200, Niklas Söderlund wrote:\n> > The self and main paths are very similar and the introduction of support\n> > for two paths simulating sly (paths) have made it clear their handling\n> \n> Is it me not knowing what \"simulating sly (paths)\" means ?\n\nI think this is a secret my spellchecker will take to the grave :-)\n\n  The self and main paths are very similar and the introduction of \n  support for two simulations streams have made it clear their \n  handling...\n\nIs what I intended to say.\n\n> \n> > could be abstracted in a separate class.\n> >\n> > This is the first step to create such an class by breaking out the\n> > initialization and storage of the video and subdevices. There is no\n> > functional change in this patch.\n> >\n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  src/libcamera/pipeline/rkisp1/meson.build    |   1 +\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 104 ++++++++-----------\n> >  src/libcamera/pipeline/rkisp1/rkisp1path.cpp |  53 ++++++++++\n> >  src/libcamera/pipeline/rkisp1/rkisp1path.h   |  46 ++++++++\n> >  4 files changed, 144 insertions(+), 60 deletions(-)\n> >  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> >  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.h\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/meson.build b/src/libcamera/pipeline/rkisp1/meson.build\n> > index 1ab3964a6db190f0..eddf795e54575956 100644\n> > --- a/src/libcamera/pipeline/rkisp1/meson.build\n> > +++ b/src/libcamera/pipeline/rkisp1/meson.build\n> > @@ -2,5 +2,6 @@\n> >\n> >  libcamera_sources += files([\n> >      'rkisp1.cpp',\n> > +    'rkisp1path.cpp',\n> >      'timeline.cpp',\n> >  ])\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 27a3c44da3805c5f..e738a7eb19264d79 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -33,6 +33,7 @@\n> >  #include \"libcamera/internal/v4l2_subdevice.h\"\n> >  #include \"libcamera/internal/v4l2_videodevice.h\"\n> >\n> > +#include \"rkisp1path.h\"\n> >  #include \"timeline.h\"\n> >\n> >  namespace libcamera {\n> > @@ -147,12 +148,11 @@ public:\n> >  class RkISP1CameraData : public CameraData\n> >  {\n> >  public:\n> > -\tRkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *mainPathVideo,\n> > -\t\t\t V4L2VideoDevice *selfPathVideo)\n> > +\tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > +\t\t\t RkISP1SelfPath *selfPath)\n> >  \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> > -\t\t  frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n> > -\t\t  selfPathVideo_(selfPathVideo), mainPathActive_(false),\n> > -\t\t  selfPathActive_(false)\n> > +\t\t  frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath),\n> > +\t\t  mainPathActive_(false), selfPathActive_(false)\n> >  \t{\n> >  \t}\n> >\n> > @@ -171,8 +171,8 @@ public:\n> >  \tRkISP1Frames frameInfo_;\n> >  \tRkISP1Timeline timeline_;\n> >\n> > -\tV4L2VideoDevice *mainPathVideo_;\n> > -\tV4L2VideoDevice *selfPathVideo_;\n> > +\tRkISP1MainPath *mainPath_;\n> > +\tRkISP1SelfPath *selfPath_;\n> >\n> >  \tbool mainPathActive_;\n> >  \tbool selfPathActive_;\n> > @@ -259,13 +259,12 @@ private:\n> >\n> >  \tMediaDevice *media_;\n> >  \tV4L2Subdevice *isp_;\n> > -\tV4L2Subdevice *mainPathResizer_;\n> > -\tV4L2Subdevice *selfPathResizer_;\n> > -\tV4L2VideoDevice *mainPathVideo_;\n> > -\tV4L2VideoDevice *selfPathVideo_;\n> >  \tV4L2VideoDevice *param_;\n> >  \tV4L2VideoDevice *stat_;\n> >\n> > +\tRkISP1MainPath mainPath_;\n> > +\tRkISP1SelfPath selfPath_;\n> > +\n> >  \tstd::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;\n> >  \tstd::vector<std::unique_ptr<FrameBuffer>> statBuffers_;\n> >  \tstd::queue<FrameBuffer *> availableParamBuffers_;\n> > @@ -441,10 +440,10 @@ protected:\n> >  \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> >\n> >  \t\tif (info->mainPathBuffer)\n> > -\t\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> > +\t\t\tpipe_->mainPath_.video_->queueBuffer(info->mainPathBuffer);\n> >\n> >  \t\tif (info->selfPathBuffer)\n> > -\t\t\tpipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);\n> > +\t\t\tpipe_->selfPath_.video_->queueBuffer(info->selfPathBuffer);\n> >  \t}\n> >\n> >  private:\n> > @@ -554,13 +553,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\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> > +\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_->selfPathVideo_);\n> > +\t\t\t    RKISP1_RSZ_SP_SRC_MAX, data_->selfPath_->video_);\n> >  }\n> >\n> >  bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > @@ -688,9 +687,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  }\n> >\n> >  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > -\t: PipelineHandler(manager), isp_(nullptr), mainPathResizer_(nullptr),\n> > -\t  selfPathResizer_(nullptr), mainPathVideo_(nullptr),\n> > -\t  selfPathVideo_(nullptr), param_(nullptr), stat_(nullptr)\n> > +\t: PipelineHandler(manager), isp_(nullptr), param_(nullptr),\n> > +\t  stat_(nullptr)\n> >  {\n> >  }\n> >\n> > @@ -698,10 +696,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n> >  {\n> >  \tdelete param_;\n> >  \tdelete stat_;\n> > -\tdelete mainPathVideo_;\n> > -\tdelete selfPathVideo_;\n> > -\tdelete mainPathResizer_;\n> > -\tdelete selfPathResizer_;\n> >  \tdelete isp_;\n> >  }\n> >\n> > @@ -821,12 +815,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >  \t\tV4L2VideoDevice *video;\n> >\n> >  \t\tif (cfg.stream() == &data->mainPathStream_) {\n> > -\t\t\tresizer = mainPathResizer_;\n> > -\t\t\tvideo = mainPathVideo_;\n> > +\t\t\tresizer = mainPath_.resizer_;\n> > +\t\t\tvideo = mainPath_.video_;\n> >  \t\t\tdata->mainPathActive_ = true;\n> >  \t\t} else {\n> > -\t\t\tresizer = selfPathResizer_;\n> > -\t\t\tvideo = selfPathVideo_;\n> > +\t\t\tresizer = selfPath_.resizer_;\n> > +\t\t\tvideo = selfPath_.video_;\n> >  \t\t\tdata->selfPathActive_ = true;\n> >  \t\t}\n> >\n> > @@ -834,7 +828,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >  \t\tif (ret < 0)\n> >  \t\t\treturn ret;\n> >\n> > -\t\tconst char *name = resizer == mainPathResizer_ ? \"main\" : \"self\";\n> > +\t\tconst char *name = resizer == mainPath_.resizer_ ? \"main\" : \"self\";\n> >\n> >  \t\tLOG(RkISP1, Debug)\n> >  \t\t\t<< \"Configured \" << name << \" resizer input pad with \"\n> > @@ -894,9 +888,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> >  \tunsigned int count = stream->configuration().bufferCount;\n> >\n> >  \tif (stream == &data->mainPathStream_)\n> > -\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n> > +\t\treturn mainPath_.video_->exportBuffers(count, buffers);\n> >  \telse if (stream == &data->selfPathStream_)\n> > -\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n> > +\t\treturn selfPath_.video_->exportBuffers(count, buffers);\n> >\n> >  \treturn -EINVAL;\n> >  }\n> > @@ -913,14 +907,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> >  \t});\n> >\n> >  \tif (data->mainPathActive_) {\n> > -\t\tret = mainPathVideo_->importBuffers(\n> > +\t\tret = mainPath_.video_->importBuffers(\n> >  \t\t\tdata->mainPathStream_.configuration().bufferCount);\n> >  \t\tif (ret < 0)\n> >  \t\t\tgoto error;\n> >  \t}\n> >\n> >  \tif (data->selfPathActive_) {\n> > -\t\tret = selfPathVideo_->importBuffers(\n> > +\t\tret = selfPath_.video_->importBuffers(\n> >  \t\t\tdata->selfPathStream_.configuration().bufferCount);\n> >  \t\tif (ret < 0)\n> >  \t\t\tgoto error;\n> > @@ -955,8 +949,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> >  error:\n> >  \tparamBuffers_.clear();\n> >  \tstatBuffers_.clear();\n> > -\tmainPathVideo_->releaseBuffers();\n> > -\tselfPathVideo_->releaseBuffers();\n> > +\tmainPath_.video_->releaseBuffers();\n> > +\tselfPath_.video_->releaseBuffers();\n> >\n> >  \treturn ret;\n> >  }\n> > @@ -987,10 +981,10 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> >  \tif (stat_->releaseBuffers())\n> >  \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> >\n> > -\tif (mainPathVideo_->releaseBuffers())\n> > +\tif (mainPath_.video_->releaseBuffers())\n> >  \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n> >\n> > -\tif (selfPathVideo_->releaseBuffers())\n> > +\tif (selfPath_.video_->releaseBuffers())\n> >  \t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n> >\n> >  \treturn 0;\n> > @@ -1038,7 +1032,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >  \tstd::map<unsigned int, IPAStream> streamConfig;\n> >\n> >  \tif (data->mainPathActive_) {\n> > -\t\tret = mainPathVideo_->streamOn();\n> > +\t\tret = mainPath_.video_->streamOn();\n> >  \t\tif (ret) {\n> >  \t\t\tparam_->streamOff();\n> >  \t\t\tstat_->streamOff();\n> > @@ -1057,10 +1051,10 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >  \t}\n> >\n> >  \tif (data->selfPathActive_) {\n> > -\t\tret = selfPathVideo_->streamOn();\n> > +\t\tret = selfPath_.video_->streamOn();\n> >  \t\tif (ret) {\n> >  \t\t\tif (data->mainPathActive_)\n> > -\t\t\t\tmainPathVideo_->streamOff();\n> > +\t\t\t\tmainPath_.video_->streamOff();\n> >\n> >  \t\t\tparam_->streamOff();\n> >  \t\t\tstat_->streamOff();\n> > @@ -1106,7 +1100,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> >  \tint ret;\n> >\n> >  \tif (data->selfPathActive_) {\n> > -\t\tret = selfPathVideo_->streamOff();\n> > +\t\tret = selfPath_.video_->streamOff();\n> >  \t\tif (ret)\n> >  \t\t\tLOG(RkISP1, Warning)\n> >  \t\t\t\t<< \"Failed to stop self path for \"\n> > @@ -1114,7 +1108,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> >  \t}\n> >\n> >  \tif (data->mainPathActive_) {\n> > -\t\tret = mainPathVideo_->streamOff();\n> > +\t\tret = mainPath_.video_->streamOff();\n> >  \t\tif (ret)\n> >  \t\t\tLOG(RkISP1, Warning)\n> >  \t\t\t\t<< \"Failed to stop main path for \"\n> > @@ -1226,8 +1220,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >  \tint ret;\n> >\n> >  \tstd::unique_ptr<RkISP1CameraData> data =\n> > -\t\tstd::make_unique<RkISP1CameraData>(this, mainPathVideo_,\n> > -\t\t\t\t\t\t   selfPathVideo_);\n> > +\t\tstd::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);\n> >\n> >  \tControlInfoMap::Map ctrls;\n> >  \tctrls.emplace(std::piecewise_construct,\n> > @@ -1281,23 +1274,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >  \tif (isp_->open() < 0)\n> >  \t\treturn false;\n> >\n> > -\tmainPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_mainpath\");\n> > -\tif (mainPathResizer_->open() < 0)\n> > -\t\treturn false;\n> > -\n> > -\tselfPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_selfpath\");\n> > -\tif (selfPathResizer_->open() < 0)\n> > -\t\treturn false;\n> > -\n> >  \t/* Locate and open the capture video node. */\n> > -\tmainPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_mainpath\");\n> > -\tif (mainPathVideo_->open() < 0)\n> > -\t\treturn false;\n> > -\n> > -\tselfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_selfpath\");\n> > -\tif (selfPathVideo_->open() < 0)\n> > -\t\treturn false;\n> > -\n> >  \tstat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_stats\");\n> >  \tif (stat_->open() < 0)\n> >  \t\treturn false;\n> > @@ -1306,8 +1283,15 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >  \tif (param_->open() < 0)\n> >  \t\treturn false;\n> >\n> > -\tmainPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > -\tselfPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > +\t/* Locate and open the ISP main and self paths. */\n> > +\tif (!mainPath_.init(media_))\n> > +\t\treturn false;\n> > +\n> > +\tif (!selfPath_.init(media_))\n> > +\t\treturn false;\n> > +\n> > +\tmainPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > +\tselfPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> >  \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> >  \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> > new file mode 100644\n> > index 0000000000000000..51a75df86baaaa7b\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> > @@ -0,0 +1,53 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * rkisp1path.cpp - Rockchip ISP1 path helper\n> > + */\n> > +\n> > +#include \"rkisp1path.h\"\n> > +\n> > +#include \"libcamera/internal/media_device.h\"\n> > +#include \"libcamera/internal/v4l2_subdevice.h\"\n> > +#include \"libcamera/internal/v4l2_videodevice.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +RkISP1Path::RkISP1Path(const char *name)\n> > +\t: resizer_(nullptr), video_(nullptr), name_(name)\n> > +{\n> > +}\n> > +\n> > +RkISP1Path::~RkISP1Path()\n> > +{\n> > +\tdelete video_;\n> > +\tdelete resizer_;\n> > +}\n> > +\n> > +bool RkISP1Path::init(MediaDevice *media)\n> > +{\n> > +\tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> > +\tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> > +\n> > +\tresizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> > +\tif (resizer_->open() < 0)\n> > +\t\treturn false;\n> > +\n> > +\tvideo_ = V4L2VideoDevice::fromEntityName(media, video);\n> > +\tif (video_->open() < 0)\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +RkISP1MainPath::RkISP1MainPath()\n> > +\t: RkISP1Path(\"main\")\n> > +{\n> > +}\n> > +\n> > +RkISP1SelfPath::RkISP1SelfPath()\n> > +\t: RkISP1Path(\"self\")\n> > +{\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> > new file mode 100644\n> > index 0000000000000000..d3172e228a3f67bf\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> > @@ -0,0 +1,46 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * rkisp1path.h - Rockchip ISP1 path helper\n> > + */\n> > +#ifndef __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> > +#define __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> > +\n> > +namespace libcamera {\n> > +\n> > +class MediaDevice;\n> > +class V4L2Subdevice;\n> > +class V4L2VideoDevice;\n> > +\n> > +class RkISP1Path\n> > +{\n> > +public:\n> > +\tRkISP1Path(const char *name);\n> > +\t~RkISP1Path();\n> > +\n> > +\tbool init(MediaDevice *media);\n> > +\n> > +\t/* \\todo Make resizer and video private. */\n> \n> What is blocking this ?\n\nFurther work later in this series, fear not by 21/22 both are private \n:-)\n\n> \n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> Thanks\n>   j\n> \n> > +\tV4L2Subdevice *resizer_;\n> > +\tV4L2VideoDevice *video_;\n> > +\n> > +private:\n> > +\tconst char *name_;\n> > +};\n> > +\n> > +class RkISP1MainPath : public RkISP1Path\n> > +{\n> > +public:\n> > +\tRkISP1MainPath();\n> > +};\n> > +\n> > +class RkISP1SelfPath : public RkISP1Path\n> > +{\n> > +public:\n> > +\tRkISP1SelfPath();\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_PIPELINE_RKISP1_PATH_H__ */\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 38C57C3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 25 Sep 2020 16:53:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B070A6303C;\n\tFri, 25 Sep 2020 18:53:21 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3428962FD8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Sep 2020 18:53:21 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id u21so3026018ljl.6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Sep 2020 09:53:21 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tl1sm2685854lfc.287.2020.09.25.09.53.19\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 25 Sep 2020 09:53:19 -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=\"R+Y86z8B\"; 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=2ELv7aByQ+moumOaBJ7MevbynVvHHlz/5ZRuZCUPOWA=;\n\tb=R+Y86z8BQ8xU8UhX8Hi9Ie1xWmqlZyFi++AIQxWKnuhXyr7G3/ij3Nd/9VZPWp17Pe\n\tSxHM3XRWomeDIMpgf5PMrLbGDSOQoAR/90MJB9NhrNlvzKaO7ZwBapOiYjLSYpRMdDTP\n\tGFZNbpI351UgSrcV6+Mi8Q+a/6I4KvOPHzbWXGoWPnriVZD72od1RxEl5/g4L5UI3Rg5\n\t1GJkdm9/C0ENicuH/xh4ZS41p2p95YB2zO7Komp5HHTfUyFL+3YraeganEczmhfSmni3\n\tVch8kaFqDO8lK26zPhCrsLsp4Ly0zpKz5TxLDpT8ulTs5QvLdU4uNjOVvI14WTgSOX6W\n\tIUfw==","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=2ELv7aByQ+moumOaBJ7MevbynVvHHlz/5ZRuZCUPOWA=;\n\tb=lo1CgUzAJNzdvwtFqC8Yt4e4OJJA2hp3wv9pnE0M3m9eCHiHBW7r62w+kQHtezOoqs\n\t0CgsPTY5+mvyY1QMMRua7T4BpdMWsgYGegzbTXF5f0bbDnduZSgVWrRhWvCN0J+dwoka\n\tdHFVmDD8yQtWpnX9ZwJLC74o592IeK+Mz5bwcq/NP2kgSw/IpcMKoZbwvabMkRtwZuO9\n\tMQv4+E6JNF3aiBOwgUJyftfRPLCNs9SULmqQagyWenhmq/ypYBLRdCZWKrK2dQIsQilT\n\tIdIYdvZJfGumINrywP51ibo6HuT2SCxWAVPpDg+gejD8qL7vJ3DvBkXsK0FWgRoeefz6\n\toyig==","X-Gm-Message-State":"AOAM532jp1JPHW5e2+9wZb+xXggn7MFrU1C4afxw+W38+mUGzDvzI4fF\n\tCY+axgDktFsXn1JrU0HqJqfHWg==","X-Google-Smtp-Source":"ABdhPJx1QErtuzKH43sHBkfLB9TMv8YXRUG1vO/CDA4SuS4S9jIc2VlKhJVOk5hSz1RpMZyfhKjXmw==","X-Received":"by 2002:a05:651c:c8:: with SMTP id\n\t8mr1537784ljr.251.1601052800352; \n\tFri, 25 Sep 2020 09:53:20 -0700 (PDT)","Date":"Fri, 25 Sep 2020 18:53:19 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200925165319.GH1757254@oden.dyn.berto.se>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-16-niklas.soderlund@ragnatech.se>\n\t<20200925151203.dpo4gcnutun7gf4c@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925151203.dpo4gcnutun7gf4c@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","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":12796,"web_url":"https://patchwork.libcamera.org/comment/12796/","msgid":"<20200928084022.whjkh7a7i6y5pimi@uno.localdomain>","date":"2020-09-28T08:40:22","subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Sep 25, 2020 at 06:53:19PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2020-09-25 17:12:03 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >   wow a long subject!\n> >\n> > On Fri, Sep 25, 2020 at 03:42:00AM +0200, Niklas Söderlund wrote:\n> > > The self and main paths are very similar and the introduction of support\n> > > for two paths simulating sly (paths) have made it clear their handling\n> >\n> > Is it me not knowing what \"simulating sly (paths)\" means ?\n>\n> I think this is a secret my spellchecker will take to the grave :-)\n>\n>   The self and main paths are very similar and the introduction of\n>   support for two simulations streams have made it clear their\n>   handling...\n>\n> Is what I intended to say.\n\nMaybe with\ns/simulations/simultaneous ?\n\n>\n> >\n> > > could be abstracted in a separate class.\n> > >\n> > > This is the first step to create such an class by breaking out the\n> > > initialization and storage of the video and subdevices. There is no\n> > > functional change in this patch.\n> > >\n> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > ---\n> > >  src/libcamera/pipeline/rkisp1/meson.build    |   1 +\n> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 104 ++++++++-----------\n> > >  src/libcamera/pipeline/rkisp1/rkisp1path.cpp |  53 ++++++++++\n> > >  src/libcamera/pipeline/rkisp1/rkisp1path.h   |  46 ++++++++\n> > >  4 files changed, 144 insertions(+), 60 deletions(-)\n> > >  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> > >  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.h\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/meson.build b/src/libcamera/pipeline/rkisp1/meson.build\n> > > index 1ab3964a6db190f0..eddf795e54575956 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/meson.build\n> > > +++ b/src/libcamera/pipeline/rkisp1/meson.build\n> > > @@ -2,5 +2,6 @@\n> > >\n> > >  libcamera_sources += files([\n> > >      'rkisp1.cpp',\n> > > +    'rkisp1path.cpp',\n> > >      'timeline.cpp',\n> > >  ])\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 27a3c44da3805c5f..e738a7eb19264d79 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -33,6 +33,7 @@\n> > >  #include \"libcamera/internal/v4l2_subdevice.h\"\n> > >  #include \"libcamera/internal/v4l2_videodevice.h\"\n> > >\n> > > +#include \"rkisp1path.h\"\n> > >  #include \"timeline.h\"\n> > >\n> > >  namespace libcamera {\n> > > @@ -147,12 +148,11 @@ public:\n> > >  class RkISP1CameraData : public CameraData\n> > >  {\n> > >  public:\n> > > -\tRkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *mainPathVideo,\n> > > -\t\t\t V4L2VideoDevice *selfPathVideo)\n> > > +\tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > > +\t\t\t RkISP1SelfPath *selfPath)\n> > >  \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> > > -\t\t  frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n> > > -\t\t  selfPathVideo_(selfPathVideo), mainPathActive_(false),\n> > > -\t\t  selfPathActive_(false)\n> > > +\t\t  frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath),\n> > > +\t\t  mainPathActive_(false), selfPathActive_(false)\n> > >  \t{\n> > >  \t}\n> > >\n> > > @@ -171,8 +171,8 @@ public:\n> > >  \tRkISP1Frames frameInfo_;\n> > >  \tRkISP1Timeline timeline_;\n> > >\n> > > -\tV4L2VideoDevice *mainPathVideo_;\n> > > -\tV4L2VideoDevice *selfPathVideo_;\n> > > +\tRkISP1MainPath *mainPath_;\n> > > +\tRkISP1SelfPath *selfPath_;\n> > >\n> > >  \tbool mainPathActive_;\n> > >  \tbool selfPathActive_;\n> > > @@ -259,13 +259,12 @@ private:\n> > >\n> > >  \tMediaDevice *media_;\n> > >  \tV4L2Subdevice *isp_;\n> > > -\tV4L2Subdevice *mainPathResizer_;\n> > > -\tV4L2Subdevice *selfPathResizer_;\n> > > -\tV4L2VideoDevice *mainPathVideo_;\n> > > -\tV4L2VideoDevice *selfPathVideo_;\n> > >  \tV4L2VideoDevice *param_;\n> > >  \tV4L2VideoDevice *stat_;\n> > >\n> > > +\tRkISP1MainPath mainPath_;\n> > > +\tRkISP1SelfPath selfPath_;\n> > > +\n> > >  \tstd::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;\n> > >  \tstd::vector<std::unique_ptr<FrameBuffer>> statBuffers_;\n> > >  \tstd::queue<FrameBuffer *> availableParamBuffers_;\n> > > @@ -441,10 +440,10 @@ protected:\n> > >  \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> > >\n> > >  \t\tif (info->mainPathBuffer)\n> > > -\t\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> > > +\t\t\tpipe_->mainPath_.video_->queueBuffer(info->mainPathBuffer);\n> > >\n> > >  \t\tif (info->selfPathBuffer)\n> > > -\t\t\tpipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);\n> > > +\t\t\tpipe_->selfPath_.video_->queueBuffer(info->selfPathBuffer);\n> > >  \t}\n> > >\n> > >  private:\n> > > @@ -554,13 +553,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\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> > > +\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_->selfPathVideo_);\n> > > +\t\t\t    RKISP1_RSZ_SP_SRC_MAX, data_->selfPath_->video_);\n> > >  }\n> > >\n> > >  bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > @@ -688,9 +687,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >  }\n> > >\n> > >  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > -\t: PipelineHandler(manager), isp_(nullptr), mainPathResizer_(nullptr),\n> > > -\t  selfPathResizer_(nullptr), mainPathVideo_(nullptr),\n> > > -\t  selfPathVideo_(nullptr), param_(nullptr), stat_(nullptr)\n> > > +\t: PipelineHandler(manager), isp_(nullptr), param_(nullptr),\n> > > +\t  stat_(nullptr)\n> > >  {\n> > >  }\n> > >\n> > > @@ -698,10 +696,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n> > >  {\n> > >  \tdelete param_;\n> > >  \tdelete stat_;\n> > > -\tdelete mainPathVideo_;\n> > > -\tdelete selfPathVideo_;\n> > > -\tdelete mainPathResizer_;\n> > > -\tdelete selfPathResizer_;\n> > >  \tdelete isp_;\n> > >  }\n> > >\n> > > @@ -821,12 +815,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > >  \t\tV4L2VideoDevice *video;\n> > >\n> > >  \t\tif (cfg.stream() == &data->mainPathStream_) {\n> > > -\t\t\tresizer = mainPathResizer_;\n> > > -\t\t\tvideo = mainPathVideo_;\n> > > +\t\t\tresizer = mainPath_.resizer_;\n> > > +\t\t\tvideo = mainPath_.video_;\n> > >  \t\t\tdata->mainPathActive_ = true;\n> > >  \t\t} else {\n> > > -\t\t\tresizer = selfPathResizer_;\n> > > -\t\t\tvideo = selfPathVideo_;\n> > > +\t\t\tresizer = selfPath_.resizer_;\n> > > +\t\t\tvideo = selfPath_.video_;\n> > >  \t\t\tdata->selfPathActive_ = true;\n> > >  \t\t}\n> > >\n> > > @@ -834,7 +828,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > >  \t\tif (ret < 0)\n> > >  \t\t\treturn ret;\n> > >\n> > > -\t\tconst char *name = resizer == mainPathResizer_ ? \"main\" : \"self\";\n> > > +\t\tconst char *name = resizer == mainPath_.resizer_ ? \"main\" : \"self\";\n> > >\n> > >  \t\tLOG(RkISP1, Debug)\n> > >  \t\t\t<< \"Configured \" << name << \" resizer input pad with \"\n> > > @@ -894,9 +888,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > >  \tunsigned int count = stream->configuration().bufferCount;\n> > >\n> > >  \tif (stream == &data->mainPathStream_)\n> > > -\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n> > > +\t\treturn mainPath_.video_->exportBuffers(count, buffers);\n> > >  \telse if (stream == &data->selfPathStream_)\n> > > -\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n> > > +\t\treturn selfPath_.video_->exportBuffers(count, buffers);\n> > >\n> > >  \treturn -EINVAL;\n> > >  }\n> > > @@ -913,14 +907,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> > >  \t});\n> > >\n> > >  \tif (data->mainPathActive_) {\n> > > -\t\tret = mainPathVideo_->importBuffers(\n> > > +\t\tret = mainPath_.video_->importBuffers(\n> > >  \t\t\tdata->mainPathStream_.configuration().bufferCount);\n> > >  \t\tif (ret < 0)\n> > >  \t\t\tgoto error;\n> > >  \t}\n> > >\n> > >  \tif (data->selfPathActive_) {\n> > > -\t\tret = selfPathVideo_->importBuffers(\n> > > +\t\tret = selfPath_.video_->importBuffers(\n> > >  \t\t\tdata->selfPathStream_.configuration().bufferCount);\n> > >  \t\tif (ret < 0)\n> > >  \t\t\tgoto error;\n> > > @@ -955,8 +949,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> > >  error:\n> > >  \tparamBuffers_.clear();\n> > >  \tstatBuffers_.clear();\n> > > -\tmainPathVideo_->releaseBuffers();\n> > > -\tselfPathVideo_->releaseBuffers();\n> > > +\tmainPath_.video_->releaseBuffers();\n> > > +\tselfPath_.video_->releaseBuffers();\n> > >\n> > >  \treturn ret;\n> > >  }\n> > > @@ -987,10 +981,10 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> > >  \tif (stat_->releaseBuffers())\n> > >  \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> > >\n> > > -\tif (mainPathVideo_->releaseBuffers())\n> > > +\tif (mainPath_.video_->releaseBuffers())\n> > >  \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n> > >\n> > > -\tif (selfPathVideo_->releaseBuffers())\n> > > +\tif (selfPath_.video_->releaseBuffers())\n> > >  \t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n> > >\n> > >  \treturn 0;\n> > > @@ -1038,7 +1032,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > >  \tstd::map<unsigned int, IPAStream> streamConfig;\n> > >\n> > >  \tif (data->mainPathActive_) {\n> > > -\t\tret = mainPathVideo_->streamOn();\n> > > +\t\tret = mainPath_.video_->streamOn();\n> > >  \t\tif (ret) {\n> > >  \t\t\tparam_->streamOff();\n> > >  \t\t\tstat_->streamOff();\n> > > @@ -1057,10 +1051,10 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > >  \t}\n> > >\n> > >  \tif (data->selfPathActive_) {\n> > > -\t\tret = selfPathVideo_->streamOn();\n> > > +\t\tret = selfPath_.video_->streamOn();\n> > >  \t\tif (ret) {\n> > >  \t\t\tif (data->mainPathActive_)\n> > > -\t\t\t\tmainPathVideo_->streamOff();\n> > > +\t\t\t\tmainPath_.video_->streamOff();\n> > >\n> > >  \t\t\tparam_->streamOff();\n> > >  \t\t\tstat_->streamOff();\n> > > @@ -1106,7 +1100,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> > >  \tint ret;\n> > >\n> > >  \tif (data->selfPathActive_) {\n> > > -\t\tret = selfPathVideo_->streamOff();\n> > > +\t\tret = selfPath_.video_->streamOff();\n> > >  \t\tif (ret)\n> > >  \t\t\tLOG(RkISP1, Warning)\n> > >  \t\t\t\t<< \"Failed to stop self path for \"\n> > > @@ -1114,7 +1108,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> > >  \t}\n> > >\n> > >  \tif (data->mainPathActive_) {\n> > > -\t\tret = mainPathVideo_->streamOff();\n> > > +\t\tret = mainPath_.video_->streamOff();\n> > >  \t\tif (ret)\n> > >  \t\t\tLOG(RkISP1, Warning)\n> > >  \t\t\t\t<< \"Failed to stop main path for \"\n> > > @@ -1226,8 +1220,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > >  \tint ret;\n> > >\n> > >  \tstd::unique_ptr<RkISP1CameraData> data =\n> > > -\t\tstd::make_unique<RkISP1CameraData>(this, mainPathVideo_,\n> > > -\t\t\t\t\t\t   selfPathVideo_);\n> > > +\t\tstd::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);\n> > >\n> > >  \tControlInfoMap::Map ctrls;\n> > >  \tctrls.emplace(std::piecewise_construct,\n> > > @@ -1281,23 +1274,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > >  \tif (isp_->open() < 0)\n> > >  \t\treturn false;\n> > >\n> > > -\tmainPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_mainpath\");\n> > > -\tif (mainPathResizer_->open() < 0)\n> > > -\t\treturn false;\n> > > -\n> > > -\tselfPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_selfpath\");\n> > > -\tif (selfPathResizer_->open() < 0)\n> > > -\t\treturn false;\n> > > -\n> > >  \t/* Locate and open the capture video node. */\n> > > -\tmainPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_mainpath\");\n> > > -\tif (mainPathVideo_->open() < 0)\n> > > -\t\treturn false;\n> > > -\n> > > -\tselfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_selfpath\");\n> > > -\tif (selfPathVideo_->open() < 0)\n> > > -\t\treturn false;\n> > > -\n> > >  \tstat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_stats\");\n> > >  \tif (stat_->open() < 0)\n> > >  \t\treturn false;\n> > > @@ -1306,8 +1283,15 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > >  \tif (param_->open() < 0)\n> > >  \t\treturn false;\n> > >\n> > > -\tmainPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > -\tselfPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > +\t/* Locate and open the ISP main and self paths. */\n> > > +\tif (!mainPath_.init(media_))\n> > > +\t\treturn false;\n> > > +\n> > > +\tif (!selfPath_.init(media_))\n> > > +\t\treturn false;\n> > > +\n> > > +\tmainPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > +\tselfPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > >  \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > >  \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..51a75df86baaaa7b\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> > > @@ -0,0 +1,53 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * rkisp1path.cpp - Rockchip ISP1 path helper\n> > > + */\n> > > +\n> > > +#include \"rkisp1path.h\"\n> > > +\n> > > +#include \"libcamera/internal/media_device.h\"\n> > > +#include \"libcamera/internal/v4l2_subdevice.h\"\n> > > +#include \"libcamera/internal/v4l2_videodevice.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +RkISP1Path::RkISP1Path(const char *name)\n> > > +\t: resizer_(nullptr), video_(nullptr), name_(name)\n> > > +{\n> > > +}\n> > > +\n> > > +RkISP1Path::~RkISP1Path()\n> > > +{\n> > > +\tdelete video_;\n> > > +\tdelete resizer_;\n> > > +}\n> > > +\n> > > +bool RkISP1Path::init(MediaDevice *media)\n> > > +{\n> > > +\tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> > > +\tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> > > +\n> > > +\tresizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> > > +\tif (resizer_->open() < 0)\n> > > +\t\treturn false;\n> > > +\n> > > +\tvideo_ = V4L2VideoDevice::fromEntityName(media, video);\n> > > +\tif (video_->open() < 0)\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +RkISP1MainPath::RkISP1MainPath()\n> > > +\t: RkISP1Path(\"main\")\n> > > +{\n> > > +}\n> > > +\n> > > +RkISP1SelfPath::RkISP1SelfPath()\n> > > +\t: RkISP1Path(\"self\")\n> > > +{\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> > > new file mode 100644\n> > > index 0000000000000000..d3172e228a3f67bf\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> > > @@ -0,0 +1,46 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * rkisp1path.h - Rockchip ISP1 path helper\n> > > + */\n> > > +#ifndef __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> > > +#define __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class MediaDevice;\n> > > +class V4L2Subdevice;\n> > > +class V4L2VideoDevice;\n> > > +\n> > > +class RkISP1Path\n> > > +{\n> > > +public:\n> > > +\tRkISP1Path(const char *name);\n> > > +\t~RkISP1Path();\n> > > +\n> > > +\tbool init(MediaDevice *media);\n> > > +\n> > > +\t/* \\todo Make resizer and video private. */\n> >\n> > What is blocking this ?\n>\n> Further work later in this series, fear not by 21/22 both are private\n> :-)\n>\n> >\n> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> >\n> > Thanks\n> >   j\n> >\n> > > +\tV4L2Subdevice *resizer_;\n> > > +\tV4L2VideoDevice *video_;\n> > > +\n> > > +private:\n> > > +\tconst char *name_;\n> > > +};\n> > > +\n> > > +class RkISP1MainPath : public RkISP1Path\n> > > +{\n> > > +public:\n> > > +\tRkISP1MainPath();\n> > > +};\n> > > +\n> > > +class RkISP1SelfPath : public RkISP1Path\n> > > +{\n> > > +public:\n> > > +\tRkISP1SelfPath();\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_PIPELINE_RKISP1_PATH_H__ */\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\n>\n> --\n> Regards,\n> Niklas Söderlund","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 5AC08C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Sep 2020 08:36:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDAF260BD7;\n\tMon, 28 Sep 2020 10:36:28 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D1666035F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Sep 2020 10:36:27 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 13E8A1C0013;\n\tMon, 28 Sep 2020 08:36:26 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 28 Sep 2020 10:40:22 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200928084022.whjkh7a7i6y5pimi@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-16-niklas.soderlund@ragnatech.se>\n\t<20200925151203.dpo4gcnutun7gf4c@uno.localdomain>\n\t<20200925165319.GH1757254@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925165319.GH1757254@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","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":12829,"web_url":"https://patchwork.libcamera.org/comment/12829/","msgid":"<20200928221231.GR23539@pendragon.ideasonboard.com>","date":"2020-09-28T22:12:31","subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","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:00AM +0200, Niklas Söderlund wrote:\n> The self and main paths are very similar and the introduction of support\n> for two paths simulating sly (paths) have made it clear their handling\n\nAre you using voice recognition software ? :-)\n\n> could be abstracted in a separate class.\n> \n> This is the first step to create such an class by breaking out the\n\ns/an class/a class/\n\n> initialization and storage of the video and subdevices. There is no\n> functional change in this patch.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/pipeline/rkisp1/meson.build    |   1 +\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 104 ++++++++-----------\n>  src/libcamera/pipeline/rkisp1/rkisp1path.cpp |  53 ++++++++++\n>  src/libcamera/pipeline/rkisp1/rkisp1path.h   |  46 ++++++++\n>  4 files changed, 144 insertions(+), 60 deletions(-)\n>  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n>  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.h\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/meson.build b/src/libcamera/pipeline/rkisp1/meson.build\n> index 1ab3964a6db190f0..eddf795e54575956 100644\n> --- a/src/libcamera/pipeline/rkisp1/meson.build\n> +++ b/src/libcamera/pipeline/rkisp1/meson.build\n> @@ -2,5 +2,6 @@\n>  \n>  libcamera_sources += files([\n>      'rkisp1.cpp',\n> +    'rkisp1path.cpp',\n\nMaybe rkisp1_path.cpp ?\n\n>      'timeline.cpp',\n>  ])\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 27a3c44da3805c5f..e738a7eb19264d79 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -33,6 +33,7 @@\n>  #include \"libcamera/internal/v4l2_subdevice.h\"\n>  #include \"libcamera/internal/v4l2_videodevice.h\"\n>  \n> +#include \"rkisp1path.h\"\n>  #include \"timeline.h\"\n>  \n>  namespace libcamera {\n> @@ -147,12 +148,11 @@ public:\n>  class RkISP1CameraData : public CameraData\n>  {\n>  public:\n> -\tRkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *mainPathVideo,\n> -\t\t\t V4L2VideoDevice *selfPathVideo)\n> +\tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> +\t\t\t RkISP1SelfPath *selfPath)\n>  \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> -\t\t  frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n> -\t\t  selfPathVideo_(selfPathVideo), mainPathActive_(false),\n> -\t\t  selfPathActive_(false)\n> +\t\t  frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath),\n> +\t\t  mainPathActive_(false), selfPathActive_(false)\n>  \t{\n>  \t}\n>  \n> @@ -171,8 +171,8 @@ public:\n>  \tRkISP1Frames frameInfo_;\n>  \tRkISP1Timeline timeline_;\n>  \n> -\tV4L2VideoDevice *mainPathVideo_;\n> -\tV4L2VideoDevice *selfPathVideo_;\n> +\tRkISP1MainPath *mainPath_;\n> +\tRkISP1SelfPath *selfPath_;\n>  \n>  \tbool mainPathActive_;\n>  \tbool selfPathActive_;\n> @@ -259,13 +259,12 @@ private:\n>  \n>  \tMediaDevice *media_;\n>  \tV4L2Subdevice *isp_;\n> -\tV4L2Subdevice *mainPathResizer_;\n> -\tV4L2Subdevice *selfPathResizer_;\n> -\tV4L2VideoDevice *mainPathVideo_;\n> -\tV4L2VideoDevice *selfPathVideo_;\n>  \tV4L2VideoDevice *param_;\n>  \tV4L2VideoDevice *stat_;\n>  \n> +\tRkISP1MainPath mainPath_;\n> +\tRkISP1SelfPath selfPath_;\n> +\n>  \tstd::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;\n>  \tstd::vector<std::unique_ptr<FrameBuffer>> statBuffers_;\n>  \tstd::queue<FrameBuffer *> availableParamBuffers_;\n> @@ -441,10 +440,10 @@ protected:\n>  \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n>  \n>  \t\tif (info->mainPathBuffer)\n> -\t\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> +\t\t\tpipe_->mainPath_.video_->queueBuffer(info->mainPathBuffer);\n>  \n>  \t\tif (info->selfPathBuffer)\n> -\t\t\tpipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);\n> +\t\t\tpipe_->selfPath_.video_->queueBuffer(info->selfPathBuffer);\n>  \t}\n>  \n>  private:\n> @@ -554,13 +553,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\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> +\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_->selfPathVideo_);\n> +\t\t\t    RKISP1_RSZ_SP_SRC_MAX, data_->selfPath_->video_);\n>  }\n>  \n>  bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> @@ -688,9 +687,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  }\n>  \n>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> -\t: PipelineHandler(manager), isp_(nullptr), mainPathResizer_(nullptr),\n> -\t  selfPathResizer_(nullptr), mainPathVideo_(nullptr),\n> -\t  selfPathVideo_(nullptr), param_(nullptr), stat_(nullptr)\n> +\t: PipelineHandler(manager), isp_(nullptr), param_(nullptr),\n> +\t  stat_(nullptr)\n>  {\n>  }\n>  \n> @@ -698,10 +696,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n>  {\n>  \tdelete param_;\n>  \tdelete stat_;\n> -\tdelete mainPathVideo_;\n> -\tdelete selfPathVideo_;\n> -\tdelete mainPathResizer_;\n> -\tdelete selfPathResizer_;\n>  \tdelete isp_;\n>  }\n>  \n> @@ -821,12 +815,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \t\tV4L2VideoDevice *video;\n>  \n>  \t\tif (cfg.stream() == &data->mainPathStream_) {\n> -\t\t\tresizer = mainPathResizer_;\n> -\t\t\tvideo = mainPathVideo_;\n> +\t\t\tresizer = mainPath_.resizer_;\n> +\t\t\tvideo = mainPath_.video_;\n>  \t\t\tdata->mainPathActive_ = true;\n>  \t\t} else {\n> -\t\t\tresizer = selfPathResizer_;\n> -\t\t\tvideo = selfPathVideo_;\n> +\t\t\tresizer = selfPath_.resizer_;\n> +\t\t\tvideo = selfPath_.video_;\n>  \t\t\tdata->selfPathActive_ = true;\n>  \t\t}\n>  \n> @@ -834,7 +828,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \t\tif (ret < 0)\n>  \t\t\treturn ret;\n>  \n> -\t\tconst char *name = resizer == mainPathResizer_ ? \"main\" : \"self\";\n> +\t\tconst char *name = resizer == mainPath_.resizer_ ? \"main\" : \"self\";\n>  \n>  \t\tLOG(RkISP1, Debug)\n>  \t\t\t<< \"Configured \" << name << \" resizer input pad with \"\n> @@ -894,9 +888,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n>  \tunsigned int count = stream->configuration().bufferCount;\n>  \n>  \tif (stream == &data->mainPathStream_)\n> -\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n> +\t\treturn mainPath_.video_->exportBuffers(count, buffers);\n>  \telse if (stream == &data->selfPathStream_)\n> -\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n> +\t\treturn selfPath_.video_->exportBuffers(count, buffers);\n>  \n>  \treturn -EINVAL;\n>  }\n> @@ -913,14 +907,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>  \t});\n>  \n>  \tif (data->mainPathActive_) {\n> -\t\tret = mainPathVideo_->importBuffers(\n> +\t\tret = mainPath_.video_->importBuffers(\n>  \t\t\tdata->mainPathStream_.configuration().bufferCount);\n>  \t\tif (ret < 0)\n>  \t\t\tgoto error;\n>  \t}\n>  \n>  \tif (data->selfPathActive_) {\n> -\t\tret = selfPathVideo_->importBuffers(\n> +\t\tret = selfPath_.video_->importBuffers(\n>  \t\t\tdata->selfPathStream_.configuration().bufferCount);\n>  \t\tif (ret < 0)\n>  \t\t\tgoto error;\n> @@ -955,8 +949,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>  error:\n>  \tparamBuffers_.clear();\n>  \tstatBuffers_.clear();\n> -\tmainPathVideo_->releaseBuffers();\n> -\tselfPathVideo_->releaseBuffers();\n> +\tmainPath_.video_->releaseBuffers();\n> +\tselfPath_.video_->releaseBuffers();\n>  \n>  \treturn ret;\n>  }\n> @@ -987,10 +981,10 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>  \tif (stat_->releaseBuffers())\n>  \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n>  \n> -\tif (mainPathVideo_->releaseBuffers())\n> +\tif (mainPath_.video_->releaseBuffers())\n>  \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n>  \n> -\tif (selfPathVideo_->releaseBuffers())\n> +\tif (selfPath_.video_->releaseBuffers())\n>  \t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n>  \n>  \treturn 0;\n> @@ -1038,7 +1032,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \tstd::map<unsigned int, IPAStream> streamConfig;\n>  \n>  \tif (data->mainPathActive_) {\n> -\t\tret = mainPathVideo_->streamOn();\n> +\t\tret = mainPath_.video_->streamOn();\n>  \t\tif (ret) {\n>  \t\t\tparam_->streamOff();\n>  \t\t\tstat_->streamOff();\n> @@ -1057,10 +1051,10 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \t}\n>  \n>  \tif (data->selfPathActive_) {\n> -\t\tret = selfPathVideo_->streamOn();\n> +\t\tret = selfPath_.video_->streamOn();\n>  \t\tif (ret) {\n>  \t\t\tif (data->mainPathActive_)\n> -\t\t\t\tmainPathVideo_->streamOff();\n> +\t\t\t\tmainPath_.video_->streamOff();\n>  \n>  \t\t\tparam_->streamOff();\n>  \t\t\tstat_->streamOff();\n> @@ -1106,7 +1100,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>  \tint ret;\n>  \n>  \tif (data->selfPathActive_) {\n> -\t\tret = selfPathVideo_->streamOff();\n> +\t\tret = selfPath_.video_->streamOff();\n>  \t\tif (ret)\n>  \t\t\tLOG(RkISP1, Warning)\n>  \t\t\t\t<< \"Failed to stop self path for \"\n> @@ -1114,7 +1108,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>  \t}\n>  \n>  \tif (data->mainPathActive_) {\n> -\t\tret = mainPathVideo_->streamOff();\n> +\t\tret = mainPath_.video_->streamOff();\n>  \t\tif (ret)\n>  \t\t\tLOG(RkISP1, Warning)\n>  \t\t\t\t<< \"Failed to stop main path for \"\n> @@ -1226,8 +1220,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \tint ret;\n>  \n>  \tstd::unique_ptr<RkISP1CameraData> data =\n> -\t\tstd::make_unique<RkISP1CameraData>(this, mainPathVideo_,\n> -\t\t\t\t\t\t   selfPathVideo_);\n> +\t\tstd::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);\n>  \n>  \tControlInfoMap::Map ctrls;\n>  \tctrls.emplace(std::piecewise_construct,\n> @@ -1281,23 +1274,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \tif (isp_->open() < 0)\n>  \t\treturn false;\n>  \n> -\tmainPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_mainpath\");\n> -\tif (mainPathResizer_->open() < 0)\n> -\t\treturn false;\n> -\n> -\tselfPathResizer_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_resizer_selfpath\");\n> -\tif (selfPathResizer_->open() < 0)\n> -\t\treturn false;\n> -\n>  \t/* Locate and open the capture video node. */\n\nThis comment seems outdated.\n\n> -\tmainPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_mainpath\");\n> -\tif (mainPathVideo_->open() < 0)\n> -\t\treturn false;\n> -\n> -\tselfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_selfpath\");\n> -\tif (selfPathVideo_->open() < 0)\n> -\t\treturn false;\n> -\n>  \tstat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_stats\");\n>  \tif (stat_->open() < 0)\n>  \t\treturn false;\n> @@ -1306,8 +1283,15 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \tif (param_->open() < 0)\n>  \t\treturn false;\n>  \n> -\tmainPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> -\tselfPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\t/* Locate and open the ISP main and self paths. */\n> +\tif (!mainPath_.init(media_))\n> +\t\treturn false;\n> +\n> +\tif (!selfPath_.init(media_))\n> +\t\treturn false;\n> +\n> +\tmainPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\tselfPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n>  \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n>  \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>  \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> new file mode 100644\n> index 0000000000000000..51a75df86baaaa7b\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> @@ -0,0 +1,53 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * rkisp1path.cpp - Rockchip ISP1 path helper\n> + */\n> +\n> +#include \"rkisp1path.h\"\n> +\n> +#include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/v4l2_subdevice.h\"\n> +#include \"libcamera/internal/v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +RkISP1Path::RkISP1Path(const char *name)\n> +\t: resizer_(nullptr), video_(nullptr), name_(name)\n> +{\n> +}\n> +\n> +RkISP1Path::~RkISP1Path()\n> +{\n> +\tdelete video_;\n> +\tdelete resizer_;\n\nShould these two be stored in std::unique_ptr<> ? It would make sense\nfor V4L2Subdevice::fromEntityName() to return a unique pointer actually\n(but that can be addressed separately from this series).\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +}\n> +\n> +bool RkISP1Path::init(MediaDevice *media)\n> +{\n> +\tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> +\tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> +\n> +\tresizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> +\tif (resizer_->open() < 0)\n> +\t\treturn false;\n> +\n> +\tvideo_ = V4L2VideoDevice::fromEntityName(media, video);\n> +\tif (video_->open() < 0)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +RkISP1MainPath::RkISP1MainPath()\n> +\t: RkISP1Path(\"main\")\n> +{\n> +}\n> +\n> +RkISP1SelfPath::RkISP1SelfPath()\n> +\t: RkISP1Path(\"self\")\n> +{\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> new file mode 100644\n> index 0000000000000000..d3172e228a3f67bf\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> @@ -0,0 +1,46 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * rkisp1path.h - Rockchip ISP1 path helper\n> + */\n> +#ifndef __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> +#define __LIBCAMERA_PIPELINE_RKISP1_PATH_H__\n> +\n> +namespace libcamera {\n> +\n> +class MediaDevice;\n> +class V4L2Subdevice;\n> +class V4L2VideoDevice;\n> +\n> +class RkISP1Path\n> +{\n> +public:\n> +\tRkISP1Path(const char *name);\n> +\t~RkISP1Path();\n> +\n> +\tbool init(MediaDevice *media);\n> +\n> +\t/* \\todo Make resizer and video private. */\n> +\tV4L2Subdevice *resizer_;\n> +\tV4L2VideoDevice *video_;\n> +\n> +private:\n> +\tconst char *name_;\n> +};\n> +\n> +class RkISP1MainPath : public RkISP1Path\n> +{\n> +public:\n> +\tRkISP1MainPath();\n> +};\n> +\n> +class RkISP1SelfPath : public RkISP1Path\n> +{\n> +public:\n> +\tRkISP1SelfPath();\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PIPELINE_RKISP1_PATH_H__ */","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 A9257C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Sep 2020 22:13:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D8A060BD4;\n\tTue, 29 Sep 2020 00:13:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12F4660394\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Sep 2020 00:13:07 +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 C94AEA58;\n\tTue, 29 Sep 2020 00:13:05 +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=\"T9jRKel6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601331186;\n\tbh=YghcKu+rcyBMyJ2PPodgLtbJZw8axOUue3MVvPcP0is=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T9jRKel6RD44xjhjF+JzCB2CQRSqdbiPd9xlK0lGwGZAWfDWRKvX7lhy96y8hP0oB\n\t9hCzdcFheBGs5bXJdrXP4O0g9JclAuTFBTQU/6cnnEYEzUnT6RTIbz+RemvbjUWAml\n\t7tRars8LkyrXynywS+r9IyeK4GWa67EmVu+HD0kw=","Date":"Tue, 29 Sep 2020 01:12:31 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200928221231.GR23539@pendragon.ideasonboard.com>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-16-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-16-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1:\n\tBreakout basic path handling to own class","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>"}}]