[{"id":1489,"web_url":"https://patchwork.libcamera.org/comment/1489/","msgid":"<20190419132951.GK7006@pendragon.ideasonboard.com>","date":"2019-04-19T13:29:51","subject":"Re: [libcamera-devel] [PATCH v8 3/8] libcamera: ipu3: Create camera\n\twith 2 streams","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 Fri, Apr 19, 2019 at 03:25:26PM +0200, Jacopo Mondi wrote:\n> Sub-class the Stream class with an IPU3-specific implementation and\n> create each IPU3 camera with two streams: 'output' and 'viewfinder'\n> which represent the video streams from main and secondary ImgU output\n> respectively.\n> \n> Re-work stream configuration to handle the two video streams 'output'\n\ns/Re-work/Rework/\n\n> and 'viewfinder' separately.\n> \n> As the IPU3 driver requires viewfinder and stat video nodes to be\n> started not to stall ImgU processing, configure 'output', 'viewfinder'\n> and 'stat' regardless of the user-requested active streams.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 179 +++++++++++++++++++--------\n>  1 file changed, 129 insertions(+), 50 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 7443224d4f45..46384d88dddd 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -62,7 +62,7 @@ public:\n>  \t}\n>  \n>  \tint init(MediaDevice *media, unsigned int index);\n> -\tint configureInput(const StreamConfiguration &config,\n> +\tint configureInput(const Size &size,\n>  \t\t\t   V4L2DeviceFormat *inputFormat);\n>  \tint configureOutput(ImgUOutput *output,\n>  \t\t\t    const StreamConfiguration &config);\n> @@ -112,7 +112,7 @@ public:\n>  \t}\n>  \n>  \tint init(const MediaDevice *media, unsigned int index);\n> -\tint configure(const StreamConfiguration &config,\n> +\tint configure(const Size &size,\n>  \t\t      V4L2DeviceFormat *outputFormat);\n>  \n>  \tBufferPool *exportBuffers();\n> @@ -130,6 +130,19 @@ public:\n>  \tBufferPool pool_;\n>  };\n>  \n> +class IPU3Stream : public Stream\n> +{\n> +public:\n> +\tIPU3Stream()\n> +\t\t: active_(false), device_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\tbool active_;\n> +\tstd::string name_;\n> +\tImgUDevice::ImgUOutput *device_;\n> +};\n> +\n>  class PipelineHandlerIPU3 : public PipelineHandler\n>  {\n>  public:\n> @@ -170,7 +183,8 @@ private:\n>  \t\tCIO2Device cio2_;\n>  \t\tImgUDevice *imgu_;\n>  \n> -\t\tStream stream_;\n> +\t\tIPU3Stream outStream_;\n> +\t\tIPU3Stream vfStream_;\n>  \t};\n>  \n>  \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> @@ -209,7 +223,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n>  {\n>  \tCameraConfiguration configs;\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tStreamConfiguration *config = &configs[&data->stream_];\n> +\tStreamConfiguration config = {};\n>  \n>  \t/*\n>  \t * FIXME: Soraka: the maximum resolution reported by both sensors\n> @@ -219,12 +233,20 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n>  \t *\n>  \t * \\todo Clarify ImgU alignement requirements.\n>  \t */\n> -\tconfig->width = 2560;\n> -\tconfig->height = 1920;\n> -\tconfig->pixelFormat = V4L2_PIX_FMT_NV12;\n> -\tconfig->bufferCount = IPU3_BUFFER_COUNT;\n> +\tconfig.width = 2560;\n> +\tconfig.height = 1920;\n> +\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> +\tconfig.bufferCount = IPU3_BUFFER_COUNT;\n> +\n> +\tconfigs[&data->outStream_] = config;\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"Stream '\" << data->outStream_.name_ << \"' format set to \"\n> +\t\t<< config.toString();\n>  \n> -\tLOG(IPU3, Debug) << \"Stream format set to \" << config->toString();\n> +\tconfigs[&data->vfStream_] = config;\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"Stream '\" << data->vfStream_.name_ << \"' format set to \"\n> +\t\t<< config.toString();\n>  \n>  \treturn configs;\n>  }\n> @@ -233,30 +255,52 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  \t\t\t\t\t  const CameraConfiguration &config)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> +\tIPU3Stream *outStream = &data->outStream_;\n> +\tIPU3Stream *vfStream = &data->vfStream_;\n>  \tCIO2Device *cio2 = &data->cio2_;\n>  \tImgUDevice *imgu = data->imgu_;\n> +\tSize sensorSize = {};\n>  \tint ret;\n>  \n> -\t/*\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> +\toutStream->active_ = false;\n> +\tvfStream->active_ = false;\n> +\tfor (Stream *s : config) {\n> +\t\tIPU3Stream *stream = static_cast<IPU3Stream *>(s);\n> +\t\tconst StreamConfiguration &cfg = config[stream];\n>  \n> -\tconst Size &resolution = cio2->sensor_->resolution();\n> -\tif (cfg.width > resolution.width ||\n> -\t    cfg.height > resolution.height) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Invalid stream size: larger than sensor resolution\";\n> -\t\treturn -EINVAL;\n> +\t\t/*\n> +\t\t * Verify that the requested size respects the IPU3 alignment\n> +\t\t * requirements (the image width shall be a multiple of 8\n> +\t\t * pixels and its height a multiple of 4 pixels) and the camera\n> +\t\t * maximum sizes.\n> +\t\t *\n> +\t\t * \\todo: Consider the BDS scaling factor requirements: \"the\n> +\t\t * downscaling factor must be an integer value multiple of 1/32\"\n> +\t\t */\n> +\t\tif (cfg.width % 8 || cfg.height % 4) {\n> +\t\t\tLOG(IPU3, Error)\n> +\t\t\t\t<< \"Invalid stream size: bad alignment\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tconst Size &resolution = cio2->sensor_->resolution();\n> +\t\tif (cfg.width > resolution.width ||\n> +\t\t    cfg.height > resolution.height) {\n> +\t\t\tLOG(IPU3, Error)\n> +\t\t\t\t<< \"Invalid stream size: larger than sensor resolution\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Collect the maximum width and height: IPU3 can downscale\n> +\t\t * only.\n> +\t\t */\n> +\t\tif (cfg.width > sensorSize.width)\n> +\t\t\tsensorSize.width = cfg.width;\n> +\t\tif (cfg.height > sensorSize.height)\n> +\t\t\tsensorSize.height = cfg.height;\n> +\n> +\t\tstream->active_ = true;\n>  \t}\n>  \n>  \t/*\n> @@ -272,24 +316,51 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  \t * adjusted format to be propagated to the ImgU output devices.\n>  \t */\n>  \tV4L2DeviceFormat cio2Format = {};\n> -\tret = cio2->configure(cfg, &cio2Format);\n> +\tret = cio2->configure(sensorSize, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tret = imgu->configureInput(cfg, &cio2Format);\n> +\tret = imgu->configureInput(sensorSize, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \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> +\t/* Apply the format to the configured streams output devices. */\n> +\tfor (Stream *s : config) {\n> +\t\tIPU3Stream *stream = static_cast<IPU3Stream *>(s);\n>  \n> -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\t\tret = imgu->configureOutput(stream->device_, config[stream]);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\t/*\n> +\t * As we need to set format also on the non-active streams, use\n> +\t * the configuration of the active one for that purpose (there should\n> +\t * be at least one active stream in the configuration request).\n> +\t */\n> +\tif (!outStream->active_) {\n> +\t\tret = imgu->configureOutput(outStream->device_,\n> +\t\t\t\t\t    config[vfStream]);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\tif (!vfStream->active_) {\n> +\t\tret = imgu->configureOutput(vfStream->device_,\n> +\t\t\t\t\t    config[outStream]);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n>  \n> -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> +\t/*\n> +\t * Apply the largest available format to the stat node.\n> +\t * \\todo Revise this when we'll actually use the stat node.\n> +\t */\n> +\tStreamConfiguration statConfig = {};\n> +\tstatConfig.width = cio2Format.width;\n> +\tstatConfig.height = cio2Format.height;\n> +\n> +\tret = imgu->configureOutput(&imgu->stat_, statConfig);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> @@ -404,7 +475,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tV4L2Device *output = data->imgu_->output_.dev;\n> -\tStream *stream = &data->stream_;\n> +\tIPU3Stream *stream = &data->outStream_;\n>  \n>  \t/* Queue a buffer to the ImgU output for capture. */\n>  \tBuffer *buffer = request->findBuffer(stream);\n> @@ -555,7 +626,10 @@ int PipelineHandlerIPU3::registerCameras()\n>  \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n>  \t\tstd::unique_ptr<IPU3CameraData> data =\n>  \t\t\tutils::make_unique<IPU3CameraData>(this);\n> -\t\tstd::set<Stream *> streams{ &data->stream_ };\n> +\t\tstd::set<Stream *> streams = {\n> +\t\t\t&data->outStream_,\n> +\t\t\t&data->vfStream_,\n> +\t\t};\n>  \t\tCIO2Device *cio2 = &data->cio2_;\n>  \n>  \t\tret = cio2->init(cio2MediaDev_.get(), id);\n> @@ -563,11 +637,16 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\t\tcontinue;\n>  \n>  \t\t/**\n> -\t\t * \\todo Dynamically assign ImgU devices; as of now, limit\n> -\t\t * support to two cameras only, and assign imgu0 to the first\n> -\t\t * one and imgu1 to the second.\n> +\t\t * \\todo Dynamically assign ImgU and output devices to each\n> +\t\t * stream and camera; as of now, limit support to two cameras\n> +\t\t * only, and assign imgu0 to the first one and imgu1 to the\n> +\t\t * second.\n>  \t\t */\n>  \t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> +\t\tdata->outStream_.device_ = &data->imgu_->output_;\n> +\t\tdata->outStream_.name_ = \"output\";\n> +\t\tdata->vfStream_.device_ = &data->imgu_->viewfinder_;\n> +\t\tdata->vfStream_.name_ = \"viewfinder\";\n>  \n>  \t\t/*\n>  \t\t * Connect video devices' 'bufferReady' signals to their\n> @@ -718,11 +797,11 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \n>  /**\n>   * \\brief Configure the ImgU unit input\n> - * \\param[in] config The requested stream configuration\n> + * \\param[in] size The ImgU input frame size\n>   * \\param[in] inputFormat The format to be applied to ImgU input\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int ImgUDevice::configureInput(const StreamConfiguration &config,\n> +int ImgUDevice::configureInput(const Size &size,\n>  \t\t\t       V4L2DeviceFormat *inputFormat)\n>  {\n>  \t/* Configure the ImgU input video device with the requested sizes. */\n> @@ -760,8 +839,8 @@ int ImgUDevice::configureInput(const StreamConfiguration &config,\n>  \t\t\t << rect.toString();\n>  \n>  \tV4L2SubdeviceFormat imguFormat = {};\n> -\timguFormat.width = config.width;\n> -\timguFormat.height = config.height;\n> +\timguFormat.width = size.width;\n> +\timguFormat.height = size.height;\n>  \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n>  \n>  \tret = imgu_->setFormat(PAD_INPUT, &imguFormat);\n> @@ -1074,11 +1153,11 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n>  \n>  /**\n>   * \\brief Configure the CIO2 unit\n> - * \\param[in] config The requested configuration\n> + * \\param[in] size The requested CIO2 output frame size\n>   * \\param[out] outputFormat The CIO2 unit output image format\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int CIO2Device::configure(const StreamConfiguration &config,\n> +int CIO2Device::configure(const Size &size,\n>  \t\t\t  V4L2DeviceFormat *outputFormat)\n>  {\n>  \tV4L2SubdeviceFormat sensorFormat;\n> @@ -1092,7 +1171,7 @@ int CIO2Device::configure(const StreamConfiguration &config,\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SGBRG10_1X10,\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SGRBG10_1X10,\n>  \t\t\t\t\t    MEDIA_BUS_FMT_SRGGB10_1X10 },\n> -\t\t\t\t\t  Size(config.width, config.height));\n> +\t\t\t\t\t  size);\n>  \tret = sensor_->setFormat(&sensorFormat);\n>  \tif (ret)\n>  \t\treturn ret;","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 1385560B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 19 Apr 2019 15:30:10 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5A86A31A;\n\tFri, 19 Apr 2019 15:30:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555680608;\n\tbh=CZG9XEL4CABYTIGOvMgGUZE792m2/GhLSOq1zcjp7gY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VCz/yFxx+4duXXuxKYf0CGQv2Tl9ZpYIC+IxPlJdiM7F9VB5FK9z+0CSZ6pPu7EuX\n\tmK4chKCK4EAabJ2mXN0PCONtZDXqMwHz8uRdTRrYNVzpcZrCibulHx5PXkYHIjCFVL\n\tC1dVOQq/FUg3DUBwJspglwpAV9Ctv6RR2jjT5/D0=","Date":"Fri, 19 Apr 2019 16:29:51 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190419132951.GK7006@pendragon.ideasonboard.com>","References":"<20190419132531.17856-1-jacopo@jmondi.org>\n\t<20190419132531.17856-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190419132531.17856-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v8 3/8] libcamera: ipu3: Create camera\n\twith 2 streams","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":"Fri, 19 Apr 2019 13:30:10 -0000"}}]