[{"id":1487,"web_url":"https://patchwork.libcamera.org/comment/1487/","msgid":"<20190419120840.GF7006@pendragon.ideasonboard.com>","date":"2019-04-19T12:09:04","subject":"Re: [libcamera-devel] [PATCH v7 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 12:18:34PM +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> 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> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 186 +++++++++++++++++++--------\n>  1 file changed, 134 insertions(+), 52 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 64b4210b4ce3..9b2d96f22b2b 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,20 @@ public:\n>  \tBufferPool pool_;\n>  };\n>  \n> +class IPU3Stream : public Stream\n> +{\n> +public:\n> +\tIPU3Stream()\n> +\t\t: active_(false), device_(nullptr), cfg_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\tbool active_;\n> +\tstd::string name_;\n> +\tImgUDevice::ImgUOutput *device_;\n> +\tconst StreamConfiguration *cfg_;\n> +};\n> +\n>  class PipelineHandlerIPU3 : public PipelineHandler\n>  {\n>  public:\n> @@ -170,7 +184,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 +224,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,15 +234,24 @@ 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_ << \"' set to \"\n> +\t\t<< config.width << \"x\" << config.height << \"-0x\"\n> +\t\t<< std::hex << std::setfill('0') << std::setw(8)\n> +\t\t<< config.pixelFormat;\n>  \n> +\tconfigs[&data->vfStream_] = config;\n>  \tLOG(IPU3, Debug)\n> -\t\t<< \"Stream format set to \" << config->width << \"x\"\n> -\t\t<< config->height << \"-0x\" << std::hex << std::setfill('0')\n> -\t\t<< std::setw(8) << config->pixelFormat;\n> +\t\t<< \"Stream '\" << data->vfStream_.name_ << \"' set to \"\n> +\t\t<< config.width << \"x\" << config.height << \"-0x\"\n> +\t\t<< std::hex << std::setfill('0') << std::setw(8)\n> +\t\t<< config.pixelFormat;\n>  \n>  \treturn configs;\n>  }\n> @@ -236,30 +260,53 @@ 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> +\tdata->outStream_.active_ = false;\n> +\tdata->vfStream_.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->cfg_ = &cfg;\n\nAs commented on the previous version, let's store the configuration in\nlocal variables. Otherwise we're bound to run into problems later when\nsomeone will try to access the configuration pointer after it becomes\ninvalid.\n\n> +\t\tstream->active_ = true;\n>  \t}\n>  \n>  \t/*\n> @@ -275,24 +322,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_, *stream->cfg_);\n\nFor this one a local\n\n\t\tconst StreamConfiguration &cfg = config[stream];\n\nvariable in the loop is enough.\n\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n>  \n> -\tret = imgu->configureOutput(&imgu->stat_, cfg);\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    *vfStream->cfg_);\n\nFor this, you can use config[vfStream].\n\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    *outStream->cfg_);\n\nAnd here config[outStream].\n\n(maybe with a static_cast<Stream *> if required)\n\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\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> @@ -407,7 +481,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> @@ -558,7 +632,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> @@ -566,11 +643,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> @@ -721,12 +803,12 @@ 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>   *\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> @@ -764,8 +846,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> @@ -1080,12 +1162,12 @@ 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>   *\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> @@ -1099,7 +1181,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[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C9F360B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 19 Apr 2019 14:09:14 +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 59C1131A;\n\tFri, 19 Apr 2019 14:09:13 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555675753;\n\tbh=7me5UwhsZIzU8wfpvujsQhSsvs4+pmYnEFKJ7hxEvIc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FhSDlUeYWQSrGbVvY+rZcAI4BG0kt5c6+bGv7vSonb42Dx244oRZn54GgEeIBmHoj\n\tyP+4oT2WROJfdwUVw7v28nDFZjGdkyvIOU/3jkx1HEXdOExbglM3qEnoOMMc2oIFnK\n\tmVagKpnfkkLtTPeeCg+CzaIqD9kZWH+qe4bG2qaY=","Date":"Fri, 19 Apr 2019 15:09:04 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190419120840.GF7006@pendragon.ideasonboard.com>","References":"<20190419101839.10337-1-jacopo@jmondi.org>\n\t<20190419101839.10337-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190419101839.10337-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v7 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 12:09:14 -0000"}}]