[{"id":1219,"web_url":"https://patchwork.libcamera.org/comment/1219/","msgid":"<20190402173208.GO4805@pendragon.ideasonboard.com>","date":"2019-04-02T17:32:08","subject":"Re: [libcamera-devel] [PATCH v7 06/13] 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\nOn Tue, Apr 02, 2019 at 07:13:02PM +0200, Jacopo Mondi wrote:\n> Apply the requested stream configuration to the CIO2 device, and\n> propagate formats through the the ImgU subdevice and its input and\n> output video devices.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 304 ++++++++++++++++++++++-----\n>  1 file changed, 252 insertions(+), 52 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 53e6b3b461a1..57e4bb89eaa7 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -5,6 +5,7 @@\n>   * ipu3.cpp - Pipeline handler for Intel IPU3\n>   */\n>  \n> +#include <iomanip>\n>  #include <memory>\n>  #include <vector>\n>  \n> @@ -50,22 +51,35 @@ public:\n>  \tstatic constexpr unsigned int PAD_VF = 3;\n>  \tstatic constexpr unsigned int PAD_STAT = 4;\n>  \n> +\t/* ImgU output descriptor: group data specific to an ImgU output. */\n> +\tstruct ImgUOutput {\n> +\t\tV4L2Device *dev;\n> +\t\tunsigned int pad;\n> +\t\tstd::string name;\n> +\t};\n> +\n>  \tImgUDevice()\n> -\t\t: imgu_(nullptr), input_(nullptr), output_(nullptr),\n> -\t\t  viewfinder_(nullptr), stat_(nullptr)\n> +\t\t: imgu_(nullptr), input_(nullptr)\n>  \t{\n> +\t\toutput_.dev = nullptr;\n> +\t\tviewfinder_.dev = nullptr;\n> +\t\tstat_.dev = nullptr;\n>  \t}\n>  \n>  \t~ImgUDevice()\n>  \t{\n>  \t\tdelete imgu_;\n>  \t\tdelete input_;\n> -\t\tdelete output_;\n> -\t\tdelete viewfinder_;\n> -\t\tdelete stat_;\n> +\t\tdelete output_.dev;\n> +\t\tdelete viewfinder_.dev;\n> +\t\tdelete stat_.dev;\n>  \t}\n>  \n>  \tint init(MediaDevice *media, unsigned int index);\n> +\tint configureInput(const StreamConfiguration &config,\n> +\t\t\t   V4L2DeviceFormat *inputFormat);\n> +\tint configureOutput(const ImgUOutput *output,\n> +\t\t\t    const StreamConfiguration &config);\n\nEven if this function doesn't modify the output structure as such, it\nchanges the output device configuration, so conceptually I think you\nshould drop the const keyword for the output argument.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \n>  \tunsigned int index_;\n>  \tstd::string name_;\n> @@ -73,9 +87,9 @@ public:\n>  \n>  \tV4L2Subdevice *imgu_;\n>  \tV4L2Device *input_;\n> -\tV4L2Device *output_;\n> -\tV4L2Device *viewfinder_;\n> -\tV4L2Device *stat_;\n> +\tImgUOutput output_;\n> +\tImgUOutput viewfinder_;\n> +\tImgUOutput stat_;\n>  \t/* \\todo Add param video device for 3A tuning */\n>  };\n>  \n> @@ -95,6 +109,8 @@ public:\n>  \t}\n>  \n>  \tint init(const MediaDevice *media, unsigned int index);\n> +\tint configure(const StreamConfiguration &config,\n> +\t\t      V4L2DeviceFormat *format);\n>  \n>  \tV4L2Device *output_;\n>  \tV4L2Subdevice *csi2_;\n> @@ -204,60 +220,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> +\t\t<< cfg.height << \"-0x\" << std::hex << std::setfill('0')\n> +\t\t<< std::setw(8) << cfg.pixelFormat << \" on camera '\"\n> +\t\t<< camera->name() << \"'\";\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) << \"Invalid stream size: bad alignment\";\n> +\t\treturn -EINVAL;\n> +\t}\n>  \n> -\tret = sensor->getFormat(0, &subdevFormat);\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> -\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> +\tif (cfg.width > cio2->maxSize_.width ||\n> +\t    cfg.height > cio2->maxSize_.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\t\t<< \"Invalid stream size: larger than sensor resolution\";\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> -\tret = csi2->setFormat(0, &subdevFormat);\n> +\t/*\n> +\t * Pass the requested stream size to the CIO2 unit and get back the\n> +\t * adjusted format to be propagated to the ImgU output devices.\n> +\t */\n> +\tV4L2DeviceFormat cio2Format = {};\n> +\tret = cio2->configure(cfg, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tret = cio2->getFormat(&devFormat);\n> +\tret = imgu->configureInput(cfg, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tdevFormat.width = subdevFormat.width;\n> -\tdevFormat.height = subdevFormat.height;\n> -\tdevFormat.fourcc = cfg->pixelFormat;\n> +\t/* Apply the format to the ImgU output, viewfinder and stat. */\n> +\tret = imgu->configureOutput(&imgu->output_, cfg);\n> +\tif (ret)\n> +\t\treturn ret;\n>  \n> -\tret = cio2->setFormat(&devFormat);\n> +\tret = imgu->configureOutput(&imgu->viewfinder_, 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> +\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> +\tif (ret)\n> +\t\treturn ret;\n>  \n>  \treturn 0;\n>  }\n> @@ -519,9 +537,9 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \tmedia_ = media;\n>  \n>  \t/*\n> -\t * The media entities presence has been verified by the match()\n> -\t * function, no need to check for newly created video devices\n> -\t * and subdevice validity here.\n> +\t * The media entities presence in the media device has been verified\n> +\t * by the match() function: no need to check for newly created\n> +\t * video devices and subdevice validity here.\n>  \t */\n>  \timgu_ = V4L2Subdevice::fromEntityName(media, name_);\n>  \tret = imgu_->open();\n> @@ -533,21 +551,131 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\toutput_ = V4L2Device::fromEntityName(media, name_ + \" output\");\n> -\tret = output_->open();\n> +\toutput_.dev = V4L2Device::fromEntityName(media, name_ + \" output\");\n> +\tret = output_.dev->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\toutput_.pad = PAD_OUTPUT;\n> +\toutput_.name = \"output\";\n> +\n> +\tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> +\t\t\t\t\t\t     name_ + \" viewfinder\");\n> +\tret = viewfinder_.dev->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tviewfinder_.pad = PAD_VF;\n> +\tviewfinder_.name = \"viewfinder\";\n> +\n> +\tstat_.dev = V4L2Device::fromEntityName(media, name_ + \" 3a stat\");\n> +\tret = stat_.dev->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstat_.pad = PAD_STAT;\n> +\tstat_.name = \"stat\";\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the ImgU unit input\n> + * \\param[in] config The requested stream configuration\n> + * \\param[in] inputFormat The format to be applied to ImgU input\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::configureInput(const StreamConfiguration &config,\n> +\t\t\t       V4L2DeviceFormat *inputFormat)\n> +{\n> +\t/* Configure the ImgU input video device with the requested sizes. */\n> +\tint ret = input_->setFormat(inputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"ImgU input format = \" << inputFormat->toString();\n> +\n> +\t/*\n> +\t * \\todo The IPU3 driver implementation shall be changed to use the\n> +\t * input sizes as 'ImgU Input' subdevice sizes, and use the desired\n> +\t * GDC output sizes to configure the crop/compose rectangles.\n> +\t *\n> +\t * The current IPU3 driver implementation uses GDC sizes as the\n> +\t * 'ImgU Input' subdevice sizes, and the input video device sizes\n> +\t * to configure the crop/compose rectangles, contradicting the\n> +\t * V4L2 specification.\n> +\t */\n> +\tRectangle rect = {\n> +\t\t.x = 0,\n> +\t\t.y = 0,\n> +\t\t.w = inputFormat->width,\n> +\t\t.h = inputFormat->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> -\tviewfinder_ = V4L2Device::fromEntityName(media, name_ + \" viewfinder\");\n> -\tret = viewfinder_->open();\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] output The ImgU output device to configure\n> + * \\param[in] config The requested configuration\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::configureOutput(const ImgUOutput *output,\n> +\t\t\t\tconst StreamConfiguration &config)\n> +{\n> +\tV4L2Device *dev = output->dev;\n> +\tunsigned int pad = output->pad;\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> -\tstat_ = V4L2Device::fromEntityName(media, name_ + \" 3a stat\");\n> -\tret = stat_->open();\n> +\t/* No need to apply format to the stat node. */\n> +\tif (output == &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 = dev->setFormat(&outputFormat);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> +\tLOG(IPU3, Debug) << \"ImgU \" << output->name << \" format = \"\n> +\t\t\t << outputFormat.toString();\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -645,6 +773,78 @@ 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] cio2Format The CIO2 unit output image format\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CIO2Device::configure(const StreamConfiguration &config,\n> +\t\t\t  V4L2DeviceFormat *cio2Format)\n> +{\n> +\tunsigned int imageSize = config.width * config.height;\n> +\tV4L2SubdeviceFormat sensorFormat = {};\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\t * \\todo: Unconditionally scale on the sensor as much\n> +\t\t\t * as possible. This will need to be revisited when\n> +\t\t\t * implementing the scaling policy.\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\tsensorFormat.width = size.maxWidth;\n> +\t\t\tsensorFormat.height = size.maxHeight;\n> +\t\t\tsensorFormat.mbus_code = it.first;\n> +\t\t}\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, &sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = csi2_->setFormat(0, &sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tcio2Format->width = sensorFormat.width;\n> +\tcio2Format->height = sensorFormat.height;\n> +\tcio2Format->fourcc = mediaBusToCIO2Format(sensorFormat.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[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9637610B6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2019 19:32:19 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 532262F9;\n\tTue,  2 Apr 2019 19:32:19 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554226339;\n\tbh=Bte0fV5owFPlUq71s8bSJGRnvq3cUPbXTyvkD5xy4JY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nQsNMONLLu+LgSBYZaz2lYcRzG3yXNFi0aX6IrcJOXb8xxzvjhbX0SD7tiQh48xDo\n\tnDdU+qpg63FwirBzxC5HFjwapg1IsCU4wroMaNKfX8UvJNpsRL2GJOjR8Kusp0aQZS\n\ticI19UYJVOHHoVW9wA7TlJk75QdvAHTfhZPNfVrI=","Date":"Tue, 2 Apr 2019 20:32:08 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402173208.GO4805@pendragon.ideasonboard.com>","References":"<20190402171309.6447-1-jacopo@jmondi.org>\n\t<20190402171309.6447-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190402171309.6447-7-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v7 06/13] 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 17:32:20 -0000"}},{"id":1228,"web_url":"https://patchwork.libcamera.org/comment/1228/","msgid":"<20190402182228.GC23466@bigcity.dyn.berto.se>","date":"2019-04-02T18:22:28","subject":"Re: [libcamera-devel] [PATCH v7 06/13] libcamera: ipu3: Apply image\n\tformat to the pipeline","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 work.\n\nOn 2019-04-02 19:13:02 +0200, Jacopo Mondi wrote:\n> Apply the requested stream configuration to the CIO2 device, and\n> propagate formats through the the ImgU subdevice and its input and\n> output video devices.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n\nI like the end result of this patch, nice work.\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 304 ++++++++++++++++++++++-----\n>  1 file changed, 252 insertions(+), 52 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 53e6b3b461a1..57e4bb89eaa7 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -5,6 +5,7 @@\n>   * ipu3.cpp - Pipeline handler for Intel IPU3\n>   */\n>  \n> +#include <iomanip>\n>  #include <memory>\n>  #include <vector>\n>  \n> @@ -50,22 +51,35 @@ public:\n>  \tstatic constexpr unsigned int PAD_VF = 3;\n>  \tstatic constexpr unsigned int PAD_STAT = 4;\n>  \n> +\t/* ImgU output descriptor: group data specific to an ImgU output. */\n> +\tstruct ImgUOutput {\n> +\t\tV4L2Device *dev;\n> +\t\tunsigned int pad;\n> +\t\tstd::string name;\n> +\t};\n> +\n>  \tImgUDevice()\n> -\t\t: imgu_(nullptr), input_(nullptr), output_(nullptr),\n> -\t\t  viewfinder_(nullptr), stat_(nullptr)\n> +\t\t: imgu_(nullptr), input_(nullptr)\n>  \t{\n> +\t\toutput_.dev = nullptr;\n> +\t\tviewfinder_.dev = nullptr;\n> +\t\tstat_.dev = nullptr;\n>  \t}\n>  \n>  \t~ImgUDevice()\n>  \t{\n>  \t\tdelete imgu_;\n>  \t\tdelete input_;\n> -\t\tdelete output_;\n> -\t\tdelete viewfinder_;\n> -\t\tdelete stat_;\n> +\t\tdelete output_.dev;\n> +\t\tdelete viewfinder_.dev;\n> +\t\tdelete stat_.dev;\n>  \t}\n>  \n>  \tint init(MediaDevice *media, unsigned int index);\n> +\tint configureInput(const StreamConfiguration &config,\n> +\t\t\t   V4L2DeviceFormat *inputFormat);\n> +\tint configureOutput(const ImgUOutput *output,\n> +\t\t\t    const StreamConfiguration &config);\n>  \n>  \tunsigned int index_;\n>  \tstd::string name_;\n> @@ -73,9 +87,9 @@ public:\n>  \n>  \tV4L2Subdevice *imgu_;\n>  \tV4L2Device *input_;\n> -\tV4L2Device *output_;\n> -\tV4L2Device *viewfinder_;\n> -\tV4L2Device *stat_;\n> +\tImgUOutput output_;\n> +\tImgUOutput viewfinder_;\n> +\tImgUOutput stat_;\n>  \t/* \\todo Add param video device for 3A tuning */\n>  };\n>  \n> @@ -95,6 +109,8 @@ public:\n>  \t}\n>  \n>  \tint init(const MediaDevice *media, unsigned int index);\n> +\tint configure(const StreamConfiguration &config,\n> +\t\t      V4L2DeviceFormat *format);\n>  \n>  \tV4L2Device *output_;\n>  \tV4L2Subdevice *csi2_;\n> @@ -204,60 +220,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> +\t\t<< cfg.height << \"-0x\" << std::hex << std::setfill('0')\n> +\t\t<< std::setw(8) << cfg.pixelFormat << \" on camera '\"\n> +\t\t<< camera->name() << \"'\";\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) << \"Invalid stream size: bad alignment\";\n> +\t\treturn -EINVAL;\n> +\t}\n>  \n> -\tret = sensor->getFormat(0, &subdevFormat);\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> -\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> +\tif (cfg.width > cio2->maxSize_.width ||\n> +\t    cfg.height > cio2->maxSize_.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\t\t<< \"Invalid stream size: larger than sensor resolution\";\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> -\tret = csi2->setFormat(0, &subdevFormat);\n> +\t/*\n> +\t * Pass the requested stream size to the CIO2 unit and get back the\n> +\t * adjusted format to be propagated to the ImgU output devices.\n> +\t */\n> +\tV4L2DeviceFormat cio2Format = {};\n> +\tret = cio2->configure(cfg, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tret = cio2->getFormat(&devFormat);\n> +\tret = imgu->configureInput(cfg, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tdevFormat.width = subdevFormat.width;\n> -\tdevFormat.height = subdevFormat.height;\n> -\tdevFormat.fourcc = cfg->pixelFormat;\n> +\t/* Apply the format to the ImgU output, viewfinder and stat. */\n> +\tret = imgu->configureOutput(&imgu->output_, cfg);\n> +\tif (ret)\n> +\t\treturn ret;\n>  \n> -\tret = cio2->setFormat(&devFormat);\n> +\tret = imgu->configureOutput(&imgu->viewfinder_, 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> +\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> +\tif (ret)\n> +\t\treturn ret;\n>  \n>  \treturn 0;\n>  }\n> @@ -519,9 +537,9 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \tmedia_ = media;\n>  \n>  \t/*\n> -\t * The media entities presence has been verified by the match()\n> -\t * function, no need to check for newly created video devices\n> -\t * and subdevice validity here.\n> +\t * The media entities presence in the media device has been verified\n> +\t * by the match() function: no need to check for newly created\n> +\t * video devices and subdevice validity here.\n>  \t */\n>  \timgu_ = V4L2Subdevice::fromEntityName(media, name_);\n>  \tret = imgu_->open();\n> @@ -533,21 +551,131 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\toutput_ = V4L2Device::fromEntityName(media, name_ + \" output\");\n> -\tret = output_->open();\n> +\toutput_.dev = V4L2Device::fromEntityName(media, name_ + \" output\");\n> +\tret = output_.dev->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\toutput_.pad = PAD_OUTPUT;\n> +\toutput_.name = \"output\";\n> +\n> +\tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> +\t\t\t\t\t\t     name_ + \" viewfinder\");\n> +\tret = viewfinder_.dev->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tviewfinder_.pad = PAD_VF;\n> +\tviewfinder_.name = \"viewfinder\";\n> +\n> +\tstat_.dev = V4L2Device::fromEntityName(media, name_ + \" 3a stat\");\n> +\tret = stat_.dev->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstat_.pad = PAD_STAT;\n> +\tstat_.name = \"stat\";\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the ImgU unit input\n> + * \\param[in] config The requested stream configuration\n> + * \\param[in] inputFormat The format to be applied to ImgU input\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::configureInput(const StreamConfiguration &config,\n> +\t\t\t       V4L2DeviceFormat *inputFormat)\n> +{\n> +\t/* Configure the ImgU input video device with the requested sizes. */\n> +\tint ret = input_->setFormat(inputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"ImgU input format = \" << inputFormat->toString();\n> +\n> +\t/*\n> +\t * \\todo The IPU3 driver implementation shall be changed to use the\n> +\t * input sizes as 'ImgU Input' subdevice sizes, and use the desired\n> +\t * GDC output sizes to configure the crop/compose rectangles.\n> +\t *\n> +\t * The current IPU3 driver implementation uses GDC sizes as the\n> +\t * 'ImgU Input' subdevice sizes, and the input video device sizes\n> +\t * to configure the crop/compose rectangles, contradicting the\n> +\t * V4L2 specification.\n> +\t */\n> +\tRectangle rect = {\n> +\t\t.x = 0,\n> +\t\t.y = 0,\n> +\t\t.w = inputFormat->width,\n> +\t\t.h = inputFormat->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> -\tviewfinder_ = V4L2Device::fromEntityName(media, name_ + \" viewfinder\");\n> -\tret = viewfinder_->open();\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] output The ImgU output device to configure\n> + * \\param[in] config The requested configuration\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::configureOutput(const ImgUOutput *output,\n> +\t\t\t\tconst StreamConfiguration &config)\n> +{\n> +\tV4L2Device *dev = output->dev;\n> +\tunsigned int pad = output->pad;\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> -\tstat_ = V4L2Device::fromEntityName(media, name_ + \" 3a stat\");\n> -\tret = stat_->open();\n> +\t/* No need to apply format to the stat node. */\n> +\tif (output == &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 = dev->setFormat(&outputFormat);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> +\tLOG(IPU3, Debug) << \"ImgU \" << output->name << \" format = \"\n> +\t\t\t << outputFormat.toString();\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -645,6 +773,78 @@ 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] cio2Format The CIO2 unit output image format\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CIO2Device::configure(const StreamConfiguration &config,\n> +\t\t\t  V4L2DeviceFormat *cio2Format)\n> +{\n> +\tunsigned int imageSize = config.width * config.height;\n> +\tV4L2SubdeviceFormat sensorFormat = {};\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\t * \\todo: Unconditionally scale on the sensor as much\n> +\t\t\t * as possible. This will need to be revisited when\n> +\t\t\t * implementing the scaling policy.\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\tsensorFormat.width = size.maxWidth;\n> +\t\t\tsensorFormat.height = size.maxHeight;\n> +\t\t\tsensorFormat.mbus_code = it.first;\n> +\t\t}\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, &sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = csi2_->setFormat(0, &sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tcio2Format->width = sensorFormat.width;\n> +\tcio2Format->height = sensorFormat.height;\n> +\tcio2Format->fourcc = mediaBusToCIO2Format(sensorFormat.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 */\n> -- \n> 2.21.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":"<niklas.soderlund@ragnatech.se>","Received":["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 3685E610B6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2019 20:22:30 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id l7so12477935ljg.6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 02 Apr 2019 11:22:30 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ti66sm2861051lji.43.2019.04.02.11.22.28\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 02 Apr 2019 11:22:29 -0700 (PDT)"],"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\t:user-agent; bh=6BRHKN8yKdDiSF2BCgwtQxsrDDMuHPdje2k8TxIAiWE=;\n\tb=lTXxMutIXt4XIRhfs3xvTg+Ws06MYRTPA1n0dtsa9FQCY5nm9Vf4OSPGURzL02OMk1\n\tsAuPZcaD9jihcwZdQGR2FsF0zQsBqho9HdG5W/i3+4Lki2xa11GoaW5VInnWZ3V4IKpZ\n\tro288H67D89zvNUJhm8dIg0rPmJ4FdDSHud6ng6aUT6yJ9DXmkugb/Ry7QJikdzC/T2b\n\t9MZzNnfkjyfxplztfEWD2gY07ML9mY1uhCRj0Asj02R4eCMpMQIDTWJu560LDiOrMQtf\n\tsjgiVN162hgGKfHJEGUVUo2Mu0T5UwuhPOIgM8kXzDtTQ9Fn9MvDzuMU+jMDjLs3lAHQ\n\tP+IA==","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:user-agent;\n\tbh=6BRHKN8yKdDiSF2BCgwtQxsrDDMuHPdje2k8TxIAiWE=;\n\tb=gTaf2S5KozgGpaK5dwvDo20WeOip58NUtfETysJW4Gv+m5aglwI4rIaNN4TYPSCXJ+\n\tadEnglUyro64AFlS269DIB6s8emPI/2l7YohbEgUppPMtOvmCadUV0eWEZe87LKfrqPp\n\tApdjbh0EVeuz1j4jZOpdetbV8lc78Je0Y3WO/zFHx2m2EA1+6npXHCEVDolvumwrwaz9\n\twYQ0NUxFFJe0VzL7WQmXz4zIbKxiuEhrO8ji5eFZQQF4ZvCSVBky74ulRFdFaH1CUFVl\n\tTpDTONbd59uxA4Gj41VwD4sAe4cAr9zox4+1Ee22G+yaYOYr/u/L12O0nJvhTWVx/wfp\n\tAwSQ==","X-Gm-Message-State":"APjAAAXpG0EFcoRQER/1YVJWep3PITBfPQMvqJnNqM5HMf1dmb6i6lXB\n\teOBjHMAYZlQSFN7TVlMBxDuuaxjSQUk=","X-Google-Smtp-Source":"APXvYqxdCb9vqrU/sqEpv/ODitWL+dFEunkFAtgBwJ2dYqk+qsbStav8lwivCZRLHV+eZXsmjzhzjg==","X-Received":"by 2002:a2e:9bc7:: with SMTP id w7mr34296805ljj.58.1554229349595;\n\tTue, 02 Apr 2019 11:22:29 -0700 (PDT)","Date":"Tue, 2 Apr 2019 20:22:28 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402182228.GC23466@bigcity.dyn.berto.se>","References":"<20190402171309.6447-1-jacopo@jmondi.org>\n\t<20190402171309.6447-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190402171309.6447-7-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v7 06/13] 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 18:22:30 -0000"}}]