[{"id":1343,"web_url":"https://patchwork.libcamera.org/comment/1343/","msgid":"<20190415110833.GC4809@pendragon.ideasonboard.com>","date":"2019-04-15T11:08:33","subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: ipu3: Create\n\tcamera with 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 Tue, Apr 09, 2019 at 09:25:38PM +0200, Jacopo Mondi wrote:\n> Create each IPU3 camera with two streams: 'output' and 'viewfinder'\n> which represents the video stream 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: for this reason configure\n\ns/As the/The/ or s/: for this reason/,/\n\n> 'output', 'viewfinder' and 'stat' regardless of the user requested active\n\ns/user requested/user-requested/\n\n> streams.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 180 +++++++++++++++++++--------\n>  1 file changed, 125 insertions(+), 55 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 00907bb53891..712e57c5a459 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -61,7 +61,7 @@ public:\n>  \t}\n>  \n>  \tint init(MediaDevice *media, unsigned int index);\n> -\tint configureInput(const StreamConfiguration &config,\n> +\tint configureInput(const Size &config,\n>  \t\t\t   V4L2DeviceFormat *inputFormat);\n>  \tint configureOutput(ImgUOutput *output,\n>  \t\t\t    const StreamConfiguration &config);\n> @@ -111,7 +111,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 &config,\n>  \t\t      V4L2DeviceFormat *outputFormat);\n>  \n>  \tBufferPool *exportBuffers();\n> @@ -137,12 +137,14 @@ class IPU3Stream : public Stream\n>  {\n>  public:\n>  \tIPU3Stream()\n> -\t\t: active_(false)\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> @@ -183,7 +185,8 @@ private:\n>  \t\tCIO2Device cio2_;\n>  \t\tImgUDevice *imgu_;\n>  \n> -\t\tIPU3Stream stream_;\n> +\t\tIPU3Stream outStream_;\n> +\t\tIPU3Stream vfStream_;\n>  \t};\n>  \n>  \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> @@ -222,7 +225,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> @@ -232,15 +235,22 @@ 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 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 'output' 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> +\n> +\tconfigs[&data->vfStream_] = config;\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"Stream 'viewfinder' format set to \" << config.width << \"x\"\n\nShouldn't this be 'output' instead of 'viewfinder' as you set the\nconfiguration for data->outStream_ ? You could use the stream name\ninstead of a hardcoded string to fix this.\n\n> +\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> +\t\t<< std::setw(8) << config.pixelFormat;\n>  \n>  \treturn configs;\n>  }\n> @@ -249,35 +259,59 @@ 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 cio2Config = {};\n\ns/cio2Config/cio2Size/\n\nOr maybe sensorSize ?\n\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> +\toutStream->active_ = false;\n> +\tvfStream->active_ = false;\n\nThe Stream class has a StreamConfiguration member. I wonder if we could\nreuse that instead of adding an active_ field, by making the\nconfiguration invalid to signal that the stream is not active. That way\nall streams could be reset to inactive in the Camera class instead of\nmanually in the pipeline handler. This could be done on top of this\nseries too.\n\n> +\tfor (const auto &s : config) {\n\n\tfor (Stream *s : config) {\n\n> +\t\tIPU3Stream *stream = static_cast<IPU3Stream *>(s);\n> +\t\tconst StreamConfiguration &cfg = config[stream];\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> +\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\n> +\t\t * 1/32\"\n\nYou can move this to the end of the previous line.\n\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> -\tif (cfg.width > cio2->maxSize_.width ||\n> -\t    cfg.height > cio2->maxSize_.height) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Invalid stream size: larger than sensor resolution\";\n> -\t\treturn -EINVAL;\n> +\t\tif (cfg.width > cio2->maxSize_.width ||\n> +\t\t    cfg.height > cio2->maxSize_.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\tLOG(IPU3, Info)\n> +\t\t\t<< \"Stream '\" << stream->name_ << \"' \"\n> +\t\t\t<< cfg.width << \"x\" << cfg.height << \"-0x\"\n> +\t\t\t<< std::hex << std::setw(8) << cfg.pixelFormat\n> +\t\t\t<< \" on camera'\" << camera->name() << \"'\";\n\nI have a patch in my rkisp1 pipeline handler implementation that moves\nthis to the caller, as the code is common to all pipeline handlers. If\nwe want to print the stream name we need to move it to the Stream class.\nDo you think that would be useful ?\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 > cio2Config.width)\n> +\t\t\tcio2Config.width = cfg.width;\n> +\t\tif (cfg.height > cio2Config.height)\n> +\t\t\tcio2Config.height = cfg.height;\n> +\n> +\t\tstream->active_ = true;\n> +\t\tstream->cfg_ = &cfg;\n>  \t}\n>  \n>  \t/*\n> @@ -293,24 +327,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(cio2Config, &cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tret = imgu->configureInput(cfg, &cio2Format);\n> +\tret = imgu->configureInput(cio2Config, &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 (auto const &s : config) {\n\n\tfor (Stream *s : config) {\n\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> +\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> +\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> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n\nI like this much better than the previous version, thank you for the\ngreat rework.\n\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> @@ -416,7 +477,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tV4L2Device *output = data->imgu_->output_.dev;\n> -\tIPU3Stream *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> @@ -567,7 +628,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> @@ -575,11 +639,14 @@ 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->vfStream_.device_ = &data->imgu_->viewfinder_;\n>  \n>  \t\t/*\n>  \t\t * Connect video devices' 'bufferReady' signals to their\n> @@ -596,7 +663,10 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n>  \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>  \n> -\t\t/* Create and register the Camera instance. */\n> +\n\nExtra blank line.\n\n> +\t\t/* Initialize and register the Camera and its streams. */\n> +\t\tdata->outStream_.name_ = \"output\";\n> +\t\tdata->vfStream_.name_ = \"viewfinder\";\n\nI would move those two lines to the previous hunk to group all\ninitialisation of IPU3Stream (and thus remove the comment change in this\nhunk).\n\n>  \t\tstd::string cameraName = cio2->sensor_->entityName() + \" \"\n>  \t\t\t\t       + std::to_string(id);\n>  \t\tstd::shared_ptr<Camera> camera = Camera::create(this,\n> @@ -730,12 +800,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] config The requested stream sizes\n\ns/The requested stream sizes/The ImgU input frame size/\n\nLet's rename the parameter from config to size.\n\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 &config,\n>  \t\t\t       V4L2DeviceFormat *inputFormat)\n>  {\n>  \t/* Configure the ImgU input video device with the requested sizes. */\n> @@ -1096,12 +1166,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] config The requested image sizes\n\ns/config The requested stream sizes/size The CIO2 output frame size/\n\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 &config,\n>  \t\t\t  V4L2DeviceFormat *outputFormat)\n>  {\n>  \tunsigned int imageSize = config.width * config.height;","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 2C5BA600F9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 15 Apr 2019 13:08:42 +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 8D0ED333;\n\tMon, 15 Apr 2019 13:08:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555326521;\n\tbh=yRcchpdvmAE6AHCkWe8l1DHFZ656nZyUOqRSrkNmYfg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R2oBNet0HM8UGSyiOiPosujQqrtRkYpwf1PG55AVKRV0gt5KuXigRxy49y5t/IiF+\n\t5NGSE7fQQUWJvd6x6C1HKT61I3JT3dZqnIyCQ1BvnXP01pQ572vIDq5kkeRXOYUrgg\n\tfrj9oLsF55AQES5QPaLxHMhMhyNMDFXw7rOr910I=","Date":"Mon, 15 Apr 2019 14:08:33 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415110833.GC4809@pendragon.ideasonboard.com>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190409192548.20325-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: ipu3: Create\n\tcamera with 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":"Mon, 15 Apr 2019 11:08:42 -0000"}}]