[{"id":12830,"web_url":"https://patchwork.libcamera.org/comment/12830/","msgid":"<20200928221926.GS23539@pendragon.ideasonboard.com>","date":"2020-09-28T22:19:26","subject":"Re: [libcamera-devel] [PATCH v3 16/22] libcamera: pipeline: rkisp1:\n\tMove path configuration 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:01AM +0200, Niklas Söderlund wrote:\n> Move the path configuration to RkISP1Path to increase code reuse and\n> make the V4L2 subdevice resizer private to the path. There is no\n> functional change.\n> \n> Signed-off-by: Niklas Söderlund \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 55 +++-----------------\n> src/libcamera/pipeline/rkisp1/rkisp1path.cpp | 55 +++++++++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1path.h | 10 +++-\n> 3 files changed, 68 insertions(+), 52 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e738a7eb19264d79..2403eec4691b5ff6 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -810,60 +810,17 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tdata->mainPathActive_ = false;\n> \tdata->selfPathActive_ = false;\n> \tfor (const StreamConfiguration &cfg : *config) {\n> -\t\tV4L2SubdeviceFormat ispFormat = format;\n> -\t\tV4L2Subdevice *resizer;\n> -\t\tV4L2VideoDevice *video;\n> -\n> \t\tif (cfg.stream() == &data->mainPathStream_) {\n> -\t\t\tresizer = mainPath_.resizer_;\n> -\t\t\tvideo = mainPath_.video_;\n> +\t\t\tret = mainPath_.configure(cfg, format);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> \t\t\tdata->mainPathActive_ = true;\n> \t\t} else {\n> -\t\t\tresizer = selfPath_.resizer_;\n> -\t\t\tvideo = selfPath_.video_;\n> +\t\t\tret = selfPath_.configure(cfg, format);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> \t\t\tdata->selfPathActive_ = true;\n> \t\t}\n> -\n> -\t\tret = resizer->setFormat(0, &ispFormat);\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> -\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> -\t\t\t<< ispFormat.toString();\n> -\n> -\t\tispFormat.size = cfg.size;\n> -\n> -\t\tLOG(RkISP1, Debug)\n> -\t\t\t<< \"Configuring \" << name << \" resizer output pad with \"\n> -\t\t\t<< ispFormat.toString();\n> -\n> -\t\tret = resizer->setFormat(1, &ispFormat);\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> -\n> -\t\tLOG(RkISP1, Debug)\n> -\t\t\t<< \"Configured \" << name << \" resizer output pad with \"\n> -\t\t\t<< ispFormat.toString();\n> -\n> -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> -\t\tV4L2DeviceFormat outputFormat = {};\n> -\t\toutputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);\n> -\t\toutputFormat.size = cfg.size;\n> -\t\toutputFormat.planesCount = info.numPlanes();\n> -\n> -\t\tret = video->setFormat(&outputFormat);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> -\n> -\t\tif (outputFormat.size != cfg.size ||\n> -\t\t outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {\n> -\t\t\tLOG(RkISP1, Error)\n> -\t\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> -\t\t\treturn -EINVAL;\n> -\t\t}\n> \t}\n> \n> \tV4L2DeviceFormat paramFormat = {};\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> index 51a75df86baaaa7b..758580934817ed6a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> @@ -7,14 +7,18 @@\n> \n> #include \"rkisp1path.h\"\n> \n> +#include \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> +LOG_DECLARE_CATEGORY(RkISP1)\n> +\n> RkISP1Path::RkISP1Path(const char *name)\n> -\t: resizer_(nullptr), video_(nullptr), name_(name)\n> +\t: video_(nullptr), name_(name), resizer_(nullptr)\n> {\n> }\n> \n> @@ -40,6 +44,55 @@ bool RkISP1Path::init(MediaDevice *media)\n> \treturn true;\n> }\n> \n> +int RkISP1Path::configure(const StreamConfiguration &config,\n> +\t\t\t const V4L2SubdeviceFormat &inputFormat)\n> +{\n> +\tint ret;\n> +\n> +\tV4L2SubdeviceFormat ispFormat = inputFormat;\n> +\n> +\tret = resizer_->setFormat(0, &ispFormat);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tLOG(RkISP1, Debug)\n> +\t\t<< \"Configured \" << name_ << \" resizer input pad with \"\n> +\t\t<< ispFormat.toString();\n> +\n> +\tispFormat.size = config.size;\n> +\n> +\tLOG(RkISP1, Debug)\n> +\t\t<< \"Configuring \" << name_ << \" resizer output pad with \"\n> +\t\t<< ispFormat.toString();\n> +\n> +\tret = resizer_->setFormat(1, &ispFormat);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tLOG(RkISP1, Debug)\n> +\t\t<< \"Configured \" << name_ << \" resizer output pad with \"\n> +\t\t<< ispFormat.toString();\n> +\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat);\n> +\tV4L2DeviceFormat outputFormat = {};\n> +\toutputFormat.fourcc = video_->toV4L2PixelFormat(config.pixelFormat);\n> +\toutputFormat.size = config.size;\n> +\toutputFormat.planesCount = info.numPlanes();\n> +\n> +\tret = video_->setFormat(&outputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (outputFormat.size != config.size ||\n> +\t outputFormat.fourcc != video_->toV4L2PixelFormat(config.pixelFormat)) {\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Unable to configure capture in \" << config.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> RkISP1MainPath::RkISP1MainPath()\n> \t: RkISP1Path(\"main\")\n> {\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> index d3172e228a3f67bf..6eb01529d2fddb1c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> @@ -12,6 +12,8 @@ namespace libcamera {\n> class MediaDevice;\n> class V4L2Subdevice;\n> class V4L2VideoDevice;\n> +struct StreamConfiguration;\n> +struct V4L2SubdeviceFormat;\n> \n> class RkISP1Path\n> {\n> @@ -21,12 +23,16 @@ public:\n> \n> \tbool init(MediaDevice *media);\n> \n> -\t/* \\todo Make resizer and video private. */\n> -\tV4L2Subdevice *resizer_;\n> +\tint configure(const StreamConfiguration &config,\n> +\t\t const V4L2SubdeviceFormat &inputFormat);\n> +\n> +\t/* \\todo Make video private. */\n> \tV4L2VideoDevice *video_;\n> \n> private:\n> \tconst char *name_;\n> +\n> +\tV4L2Subdevice *resizer_;\n> };\n> \n> class RkISP1MainPath : public RkISP1Path","headers":{"Return-Path":"","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 E41FDC3B5B\n\tfor ;\n\tMon, 28 Sep 2020 22:20:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40B0560BF7;\n\tTue, 29 Sep 2020 00:20:03 +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 22D8F60394\n\tfor ;\n\tTue, 29 Sep 2020 00:20:02 +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 8FE94A58;\n\tTue, 29 Sep 2020 00:20:01 +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=\"nH0YRlrB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601331601;\n\tbh=7H/N32g6tsdhhiIJkL/oVrbIfkOqZwajFUlkxIqmrfQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nH0YRlrBNEdteVSPZj6SCBbCQ8qA7bIc30gdqnajgv3CK+HgWU9PHV9URmUBStFgO\n\tiGn62JDLep4F7MM0LUBhhabZVhX6dbkdp859A9gYF6r0VvojZefsaPSxRsJyei5u8J\n\tTYXJgVo32vcp/GFJNfJ09mKhAOQNq6qh8TU4Urbs=","Date":"Tue, 29 Sep 2020 01:19:26 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200928221926.GS23539@pendragon.ideasonboard.com>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-17-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-17-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 16/22] libcamera: pipeline: rkisp1:\n\tMove path configuration to RkISP1Path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}}]