[{"id":1180,"web_url":"https://patchwork.libcamera.org/comment/1180/","msgid":"<20190402100210.GI4805@pendragon.ideasonboard.com>","date":"2019-04-02T10:02:10","subject":"Re: [libcamera-devel] [PATCH v5 12/19] libcamera: ipu3: Apply image\n\tformat to the pipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nI really like the direction this is taking. There's quite a few comments\nbelow, and a bit more refactoring will be needed than for the previous\npatches, but I think you've done a good job here.\n\nOn Tue, Mar 26, 2019 at 09:38:55AM +0100, Jacopo Mondi wrote:\n> Apply the requested image format to the CIO2 device, and apply the\n> resulting adjusted one to the the ImgU subdevice and its input and\n\ns/one/format/\n\nOr maybe \"and propagate formats through the ImgU.\" ?\n\n> output video devices.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 300 +++++++++++++++++++++++----\n>  1 file changed, 265 insertions(+), 35 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 0a059b75cadf..63b84706b9b2 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -52,6 +52,20 @@ public:\n>  \tstatic constexpr unsigned int PAD_VF = 3;\n>  \tstatic constexpr unsigned int PAD_STAT = 4;\n>  \n> +\t/* ImgU output identifiers: used as indexes for the below maps. */\n\ns/indexes/indices/\n\n> +\tenum OutputId {\n> +\t\tMAIN_OUTPUT,\n> +\t\tSECONDARY_OUTPUT,\n> +\t\tSTAT,\n> +\t};\n> +\n> +\t/* ImgU output descriptor: group data specific to an ImgU output. */\n> +\tstruct outputDesc {\n\ns/outputDesc/OutputDesc/\n\n> +\t\tV4L2Device *dev;\n> +\t\tunsigned int pad;\n> +\t\tstd::string name;\n> +\t};\n\nThis gets complicated. I think it would be more readable if you renamed\nthis to ImgUVideoDevice (feel free to bikeshed on the name), and\nreplaced the V4L2Device pointers in the ImgUDevice structure with it.\nSomething along the lines of\n\n\tstruct ImgUVideoDevice {\n\t\tV4L2Device *dev;\n\t\tunsigned int pad;\n\t\tstd::string name;\n\t};\n\n\tImgUVideoDevice input_;\n\tImgUVideoDevice output_;\n\tImgUVideoDevice viewfinder_;\n\tImgUVideoDevice stat_;\n\n(you could possibly leave the input_ out of this, and then name the\nstructure ImgUOutputDevice or, better I think, just ImgUOutput)\n\n> +\n>  \tImgUDevice()\n>  \t\t: imgu_(nullptr), input_(nullptr), output_(nullptr),\n>  \t\t  viewfinder_(nullptr), stat_(nullptr)\n> @@ -67,7 +81,25 @@ public:\n>  \t\tdelete stat_;\n>  \t}\n>  \n> +\t/* Imgu output map accessors. */\n> +\tV4L2Device *outputDevice(enum OutputId id)\n> +\t{\n> +\t\treturn outputMap[id].dev;\n> +\t}\n> +\tunsigned int outputPad(enum OutputId id)\n> +\t{\n> +\t\treturn outputMap[id].pad;\n> +\t}\n> +\tconst std::string &outputName(enum OutputId id)\n> +\t{\n> +\t\treturn outputMap[id].name;\n> +\t}\n\nWith the above change, you can remove the outputMap, and these methods\nwon't be needed anymore. The OutputId enum could go too, replaced by\ndirect usage of the four ImgUVideoDevice fields.\n\n> +\n>  \tint init(MediaDevice *media, unsigned int index);\n> +\tint configureInput(const StreamConfiguration &config,\n> +\t\t\t   const V4L2SubdeviceFormat &cio2Format);\n> +\tint configureOutput(enum OutputId id,\n> +\t\t\t    const StreamConfiguration &config);\n\nThis function would take a pointer to the output device, or could\npossibly become a member of the output device structure (although in\nthat case you'd have to store a backpointer to the ImgUDevice there,\nwhich may not be worth it). This comment also applies to the other\nfunctions that operate on outputs in the next patches.\n\n>  \n>  \tunsigned int index_;\n>  \tstd::string name_;\n> @@ -79,6 +111,9 @@ public:\n>  \tV4L2Device *viewfinder_;\n>  \tV4L2Device *stat_;\n>  \t/* \\todo Add param video device for 3A tuning */\n> +\n> +\t/* ImgU output map: associate an output id with its descriptor. */\n> +\tstd::map<enum OutputId, struct outputDesc> outputMap;\n>  };\n>  \n>  class CIO2Device\n> @@ -98,6 +133,8 @@ public:\n>  \n>  \tconst std::string &name() const;\n>  \tint init(const MediaDevice *media, unsigned int index);\n> +\tint configure(const StreamConfiguration &config,\n> +\t\t      V4L2SubdeviceFormat *format);\n>  \n>  \tV4L2Device *output_;\n>  \tV4L2Subdevice *csi2_;\n> @@ -203,61 +240,62 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  \t\t\t\t\t  std::map<Stream *, StreamConfiguration> &config)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tStreamConfiguration *cfg = &config[&data->stream_];\n> -\tV4L2Subdevice *sensor = data->cio2_.sensor_;\n> -\tV4L2Subdevice *csi2 = data->cio2_.csi2_;\n> -\tV4L2Device *cio2 = data->cio2_.output_;\n> -\tV4L2SubdeviceFormat subdevFormat = {};\n> -\tV4L2DeviceFormat devFormat = {};\n> +\tconst StreamConfiguration &cfg = config[&data->stream_];\n> +\tCIO2Device *cio2 = &data->cio2_;\n> +\tImgUDevice *imgu = data->imgu_;\n>  \tint ret;\n>  \n> +\tLOG(IPU3, Info)\n> +\t\t<< \"Requested image format: \" << cfg.width << \"x\"\n\ns/format:/format/\n\n> +\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> +\t\t<< cfg.pixelFormat << \" on camera:'\" << camera->name() << \"'\";\n\ns/on camera:/on camera /\n\n> +\n>  \t/*\n> -\t * FIXME: as of now, the format gets applied to the sensor and is\n> -\t * propagated along the pipeline. It should instead be applied on the\n> -\t * capture device and the sensor format calculated accordingly.\n> +\t * Verify that the requested size respects the IPU3 alignement\n> +\t * requirements (the image width shall be a multiple of 8 pixels and\n> +\t * its height a multiple of 4 pixels) and the camera maximum sizes.\n> +\t *\n> +\t * \\todo: consider the BDS scaling factor requirements:\n> +\t * \"the downscaling factor must be an integer value multiple of 1/32\"\n>  \t */\n> +\tif (cfg.width % 8 || cfg.height % 4) {\n> +\t\tLOG(IPU3, Error) << \"Stream size not support: bad alignement\";\n\ns/support/supported/\n\nOr maybe \"Invalid stream size: bad alignment\" ?\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tSizeRange cio2MaxSize = cio2->maxSizes_.second;\n> +\tif (cfg.width > cio2MaxSize.maxWidth ||\n> +\t    cfg.height > cio2MaxSize.maxHeight) {\n> +\t\tLOG(IPU3, Error) << \"Stream size not support: too big\";\n\nAnd here \"Invalid stream size: larger than sensor resolution\" ?\n\n> +\t\treturn -EINVAL;\n> +\t}\n>  \n> -\tret = sensor->getFormat(0, &subdevFormat);\n> +\t/*\n> +\t * Pass the requested output image size to the sensor and get back the\n> +\t * adjusted one to be propagated to the to the ImgU devices.\n\ns/to the to the/to the/\n\n> +\t */\n> +\tV4L2SubdeviceFormat cio2Format = {};\n> +\tret = cio2->configure(cfg, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tsubdevFormat.width = cfg->width;\n> -\tsubdevFormat.height = cfg->height;\n> -\tret = sensor->setFormat(0, &subdevFormat);\n> +\tret = imgu->configureInput(cfg, cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\t/* Return error if the requested format cannot be applied to sensor. */\n> -\tif (subdevFormat.width != cfg->width ||\n> -\t    subdevFormat.height != cfg->height) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Failed to apply image format \"\n> -\t\t\t<< subdevFormat.width << \"x\" << subdevFormat.height\n> -\t\t\t<< \" - got: \" << cfg->width << \"x\" << cfg->height;\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tret = csi2->setFormat(0, &subdevFormat);\n> +\t/* Apply the format to the ImgU output, viewfinder and stat. */\n> +\tret = imgu->configureOutput(ImgUDevice::MAIN_OUTPUT, cfg);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tret = cio2->getFormat(&devFormat);\n> +\tret = imgu->configureOutput(ImgUDevice::SECONDARY_OUTPUT, cfg);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tdevFormat.width = subdevFormat.width;\n> -\tdevFormat.height = subdevFormat.height;\n> -\tdevFormat.fourcc = cfg->pixelFormat;\n> -\n> -\tret = cio2->setFormat(&devFormat);\n> +\tret = imgu->configureOutput(ImgUDevice::STAT, cfg);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tLOG(IPU3, Info) << cio2->driverName() << \": \"\n> -\t\t\t<< devFormat.width << \"x\" << devFormat.height\n> -\t\t\t<< \"- 0x\" << std::hex << devFormat.fourcc << \" planes: \"\n> -\t\t\t<< devFormat.planes;\n> -\n>  \treturn 0;\n>  }\n>  \n> @@ -539,16 +577,140 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> +\tstruct outputDesc desc = {};\n> +\tdesc.dev = output_;\n> +\tdesc.pad = PAD_OUTPUT;\n> +\tdesc.name = \"output\";\n> +\toutputMap[MAIN_OUTPUT] = desc;\n> +\n>  \tviewfinder_ = V4L2Device::fromEntityName(media, name_ + \" viewfinder\");\n>  \tret = viewfinder_->open();\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> +\tdesc = {};\n> +\tdesc.dev = viewfinder_;\n> +\tdesc.pad = PAD_VF;\n> +\tdesc.name = \"viewfinder\";\n> +\toutputMap[SECONDARY_OUTPUT] = desc;\n> +\n>  \tstat_ = V4L2Device::fromEntityName(media, name_ + \" 3a stat\");\n>  \tret = stat_->open();\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> +\tdesc = {};\n> +\tdesc.dev = stat_;\n> +\tdesc.pad = PAD_STAT;\n> +\tdesc.name = \"stat\";\n> +\toutputMap[STAT] = desc;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the ImgU unit input\n> + * \\param[in] config The requested GDC format configuration\n\nMaybe just \"The requested stream configuration\" ? The fact that we\nconfigure the GDC here is internal I think.\n\n> + * \\param[in] cio2Format The CIO2 output format to be applied to ImgU input\n\nI would name the parameter inputFormat and update the documentation\naccordingly, as in the future this may be used for memory-to-memory\nreprocessing, without involving the CIO2. Let's try to keep the\nCIO2Device and ImgUDevice independent, even in their respective\ndocumentation.\n\n> + *\n> + * FIXME: the IPU3 driver implementation shall be changed to use the\n> + * CIO2 output sizes as 'ImgU Input' subdevice sizes, and use the\n> + * GDC output sizes to configure the crop/compose rectangles.\n> + * The current IPU3 driver implementation uses GDC output sizes as\n> + * 'ImgU Input' sizes, and the CIO2 output sizes to configure the\n> + * crop/compose rectangles, contradicting the V4L2 specification.\n\nI would move this comment to the function, just after the \"ImgU input\nfeeder and BDS rectangle\" message.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::configureInput(const StreamConfiguration &config,\n> +\t\t\t       const V4L2SubdeviceFormat &cio2Format)\n> +{\n> +\tint ret;\n> +\n> +\t/* Configure the ImgU subdevice input pad with the requested sizes. */\n> +\tV4L2DeviceFormat inputFormat = {};\n> +\tinputFormat.width = cio2Format.width;\n> +\tinputFormat.height = cio2Format.height;\n> +\tinputFormat.fourcc = mediaBusToCIO2Format(cio2Format.mbus_code);\n> +\tinputFormat.planesCount = 1;\n> +\n> +\tret = input_->setFormat(&inputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"ImgU input format = \" << inputFormat.toString();\n> +\n> +\tRectangle rect = {\n> +\t\t.x = 0,\n> +\t\t.y = 0,\n> +\t\t.w = cio2Format.width,\n> +\t\t.h = cio2Format.height,\n> +\t};\n> +\tret = imgu_->setCrop(PAD_INPUT, &rect);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = imgu_->setCompose(PAD_INPUT, &rect);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"ImgU input feeder and BDS rectangle = \"\n> +\t\t\t << rect.toString();\n> +\n> +\tV4L2SubdeviceFormat imguFormat = {};\n> +\timguFormat.width = config.width;\n> +\timguFormat.height = config.height;\n> +\timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> +\n> +\tret = imgu_->setFormat(PAD_INPUT, &imguFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"ImgU GDC format = \" << imguFormat.toString();\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the ImgU unit \\a id video output\n> + * \\param[in] id The output device node identifier\n> + * \\param[in] config The requested configuration\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::configureOutput(enum OutputId id,\n> +\t\t\t\tconst StreamConfiguration &config)\n> +{\n> +\tV4L2Device *output = outputDevice(id);\n> +\tunsigned int pad = outputPad(id);\n> +\tconst std::string name = outputName(id);\n> +\n> +\tV4L2SubdeviceFormat imguFormat = {};\n> +\timguFormat.width = config.width;\n> +\timguFormat.height = config.height;\n> +\timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> +\n> +\tint ret = imgu_->setFormat(pad, &imguFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* No need to apply format to the stat node. */\n> +\tif (id == STAT)\n> +\t\treturn 0;\n> +\n> +\tV4L2DeviceFormat outputFormat = {};\n> +\toutputFormat.width = config.width;\n> +\toutputFormat.height = config.height;\n> +\toutputFormat.fourcc = V4L2_PIX_FMT_NV12;\n> +\toutputFormat.planesCount = 2;\n> +\n> +\tret = output->setFormat(&outputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"ImgU \" << name << \" format = \"\n> +\t\t\t << outputFormat.toString();\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -650,6 +812,74 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n>  \treturn 0;\n>  }\n>  \n> +/**\n> + * \\brief Configure the CIO2 unit\n> + * \\param[in] config The requested configuration\n> + * \\param[out] format The CIO2 unit output image format to be applied on ImgU\n\nI'd remove \"to be applied on ImgU\", to keep CIO2Device self-contained.\nIn theory the caller could do anything with the format, it doesn't have\nto involve the ImgU (I'm thinking for instance about raw capture, when\nwe'll support that).\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CIO2Device::configure(const StreamConfiguration &config,\n> +\t\t\t  V4L2SubdeviceFormat *format)\n\nI think it would make more sense to return a V4L2DeviceFormat instead of\na V4L2SubdeviceFormat, as you want to tell what is captured to memory.\n\n> +{\n> +\tunsigned int imageSize = config.width * config.height;\n> +\tunsigned int best = ~0;\n> +\tint ret;\n> +\n> +\tfor (auto it : sensor_->formats(0)) {\n> +\t\t/* Only consider formats consumable by the CIO2 unit. */\n> +\t\tif (mediaBusToCIO2Format(it.first) < 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (const SizeRange &size : it.second) {\n> +\t\t\t/*\n> +\t\t\t * Only select formats bigger than the requested sizes\n> +\t\t\t * as the IPU3 cannot up-scale.\n> +\t\t\t */\n> +\t\t\tif (size.maxWidth < config.width ||\n> +\t\t\t    size.maxHeight < config.height)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tunsigned int diff = size.maxWidth * size.maxHeight\n> +\t\t\t\t\t  - imageSize;\n> +\t\t\tif (diff >= best)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tbest = diff;\n> +\n> +\t\t\tformat->width = size.maxWidth;\n> +\t\t\tformat->height = size.maxHeight;\n> +\t\t\tformat->mbus_code = it.first;\n> +\t\t}\n\nSo you scale on the sensor as much as possible instead of scaling only\nin the ImgU. As discussed previously, there are pros and cons, and for\nnow this is fine, but I would add a comment to state this explicitly.\nSomething along the lines of\n\n\"Unconditionally scale on the sensor as much as possible. This will need\nto be revisited when implementing the scaling policy.\"\n\n> +\t}\n> +\n> +\t/*\n> +\t * Apply the selected format to the sensor, the CSI-2 receiver and\n> +\t * the CIO2 output device.\n> +\t */\n> +\tret = sensor_->setFormat(0, format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = csi2_->setFormat(0, format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tV4L2DeviceFormat cio2Format = {};\n> +\tcio2Format.width = format->width;\n> +\tcio2Format.height = format->height;\n> +\tcio2Format.fourcc = mediaBusToCIO2Format(format->mbus_code);\n> +\tcio2Format.planesCount = 1;\n> +\n> +\tret = output_->setFormat(&cio2Format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"CIO2 output format = \" << cio2Format.toString();\n> +\n> +\treturn 0;\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n>  \n>  } /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8488C600FB\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2019 12:02:21 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B56A62F9;\n\tTue,  2 Apr 2019 12:02:20 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554199341;\n\tbh=XPqH8fX7Nb8YmchUXNYfRnKOrz4TqznxCjlZaDgjS08=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mlE38VWR9MK9+rFCv4CJW94f/LDSDINLa6gMNM8RXTvmQVSUqnaJ9jXt9RmWlQy8D\n\toREqbZfeOHn9wHQL5uJzvX3krscuwq2PPBK2iLNuYgmy9+Xucvd/mVtpXHjDMrrLlQ\n\tHJZc9LhJSZ6ycUW1gTu3FbRlQW5BcQMCPxnDgoSY=","Date":"Tue, 2 Apr 2019 13:02:10 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402100210.GI4805@pendragon.ideasonboard.com>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-13-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190326083902.26121-13-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 12/19] libcamera: ipu3: Apply image\n\tformat to the pipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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>","X-List-Received-Date":"Tue, 02 Apr 2019 10:02:21 -0000"}}]