[{"id":1275,"web_url":"https://patchwork.libcamera.org/comment/1275/","msgid":"<20190405112247.GX23466@bigcity.dyn.berto.se>","date":"2019-04-05T11:22:47","subject":"Re: [libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera\n\twith 2 streams","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 patch.\n\nOn 2019-04-03 17:07:29 +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, keep track of which streams have\n> been requested by the application, and configure 'output', 'viewfinder'\n> and 'stat' regardless of the user requests.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------\n> 1 file changed, 170 insertions(+), 46 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 164e187c769d..caf1051c58ab 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -171,17 +171,61 @@ private:\n> \t\tCIO2Device cio2_;\n> \t\tImgUDevice *imgu_;\n> \n> -\t\tStream stream_;\n> +\t\tStream outStream_;\n> +\t\tStream vfStream_;\n> +\n> +\t\tunsigned int activeStreamsMask;\n> \t};\n> \n> \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> \n> +\tstatic constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);\n> +\tstatic constexpr unsigned int IPU3_STREAM_VF = BIT(1);\n> +\n> \tIPU3CameraData *cameraData(const Camera *camera)\n> \t{\n> \t\treturn static_cast(\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> \n> +\tbool isOutput(IPU3CameraData *data, Stream *stream)\n> +\t{\n> +\t\treturn &data->outStream_ == stream;\n> +\t}\n> +\tbool isOutputActive(IPU3CameraData *data)\n> +\t{\n> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?\n> +\t\t\ttrue : false;\n> +\t}\n> +\tvoid setOutputActive(IPU3CameraData *data)\n> +\t{\n> +\t\tdata->activeStreamsMask |= IPU3_STREAM_OUTPUT;\n> +\t}\n> +\tbool isViewfinder(IPU3CameraData *data, Stream *stream)\n> +\t{\n> +\t\treturn &data->vfStream_ == stream;\n> +\t}\n> +\tbool isViewfinderActive(IPU3CameraData *data)\n> +\t{\n> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_VF) ?\n> +\t\t\ttrue : false;\n> +\t}\n> +\tvoid setViewfinderActive(IPU3CameraData *data)\n> +\t{\n> +\t\tdata->activeStreamsMask |= IPU3_STREAM_VF;\n> +\t}\n> +\tbool isStreamActive(IPU3CameraData *data, Stream *stream)\n> +\t{\n> +\t\tif (isOutput(data, stream) &&\n> +\t\t isOutputActive(data))\n> +\t\t\treturn true;\n> +\t\tif (isViewfinder(data, stream) &&\n> +\t\t isViewfinderActive(data))\n> +\t\t\treturn true;\n> +\n> +\t\treturn false;\n> +\t}\n> +\n\nNit picking, I find the lack of new lines between functions above hard \nto read :-) Also maybe you should add this new bit field and the helper \nfunctions in a separate patch.\n\n> \tint registerCameras();\n> \n> \tImgUDevice imgu0_;\n> @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> {\n> \tstd::map 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> @@ -220,52 +264,93 @@ 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 '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 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 'viewfinder' 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\nThis is not correct as streamConfiguration() would always return a map \nwith 2 elements even if only one stream is requested. Looking at your \nprivate branch the top commit which make use of the stream usages \n(stream roles in your case is it's based on the RFC) solves this in a \ncorrect way. Maybe you should fold that change in here and depend on the \nstream usage serries?\n\n> \n> \treturn configs;\n> }\n> \n> int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> -\t\t\t\t\t std::map &config)\n> +\t\t\t\t\t std::map +\t\t\t\t\t\t StreamConfiguration> &config)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> +\tStreamConfiguration CIO2Config = {};\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> +\t/* Remove previously configured stream masks to store the new ones. */\n> +\tdata->activeStreamsMask = 0;\n> +\tfor (auto const &streamConfig : config) {\n> +\t\tStream *stream = streamConfig.first;\n> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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> +\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<< \"Requested image format \" << cfg.width << \"x\"\n> +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> +\t\t\t<< camera->name() << \"'\";\n> +\n> +\t\t/*\n> +\t\t * FIXME: As viewfinder should be operated even when\n> +\t\t * applications do not intend to use it, we need to keep track\n> +\t\t * of which streams have to be configured, to make meaningful\n> +\t\t * decisions at configure and request queueing time.\n> +\t\t *\n> +\t\t * Walk here all the streams to configure and collect the\n> +\t\t * active ones in a bitmaks.\n> +\t\t */\n> +\t\tif (isOutput(data, stream))\n> +\t\t\tsetOutputActive(data);\n> +\t\tif (isViewfinder(data, stream))\n> +\t\t\tsetViewfinderActive(data);\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> \t}\n> \n> \t/*\n> @@ -281,26 +366,62 @@ 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> +\tfor (auto const &streamConfig : config) {\n> +\t\tStream *stream = streamConfig.first;\n> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> \n> -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\t\tif (isOutput(data, stream)) {\n> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> \n> -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\n> +\t\t\tif (isViewfinderActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Configure viewfinder using the output stream\n> +\t\t\t * configuration if it is not part of the active\n> +\t\t\t * streams list.\n> +\t\t\t */\n> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t} else if (isViewfinder(data, stream)) {\n> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tif (isOutputActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Configure output using the viewfinder stream\n> +\t\t\t * configuration if it is not part of the active\n> +\t\t\t * streams list.\n> +\t\t\t */\n> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\t}\n\nI see the need for the 'if (is*Active(data)) continue' logic but I \nwinder if you could not be a bit more clever about it :-)\n\nHow about in the for (auto const &streamConfig : config) loop only \ncreating a cfg_output and cfg_viewfinder and outside the loop call \nimgu->configureOutpu() once for each stream?\n\n> \n> \treturn 0;\n> }\n> @@ -404,7 +525,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> +\tStream *stream = &data->outStream_;\n> \n> \t/* Queue a buffer to the ImgU output for capture. */\n> \tBuffer *buffer = request->findBuffer(stream);\n> @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> \t\tstd::unique_ptr data =\n> \t\t\tutils::make_unique(this);\n> -\t\tstd::set streams{ &data->stream_ };\n> +\t\tstd::set 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> -- \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":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 876DB60DB3\n\tfor ;\n\tFri, 5 Apr 2019 13:22:49 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id v13so4914625ljk.4\n\tfor ;\n\tFri, 05 Apr 2019 04:22:49 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\te64sm4737801ljf.75.2019.04.05.04.22.48\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 05 Apr 2019 04:22:48 -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=mCaVwYsQHLWSnl0pdgDZ7sFBvQfrpTH6jYQd45Rzw3o=;\n\tb=F9pvQwjH3katZTHSqKVWY3Eb81aCw1U4pbE8Bruvcy+nAzTWUjD19UGPu46mx43fRE\n\twumG71kQ+geBJs5S8YQhpHHQwVWaQmiKv0GsNbzDKZAALUVJfkGgptau1lqitc9J3saf\n\teENO0Kdd6pAm74TB3tLbtDCBXA0u+rDVWKwe70KRUUaw11qRBLfASF8nzETkEh75l9T+\n\tF4FtfJKM400xeAgk/K1x1QiVjAERlt1XMGxws1oiQhtSZP+4mO0mByTaDV1N4mrgFVmf\n\tIIsccenygXSlCe7ezktVm0PzHokBQqQ7ZLIBdyvlWQ3RxRmZdyR63r64C4w9V6tKpcgp\n\t1YBQ==","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=mCaVwYsQHLWSnl0pdgDZ7sFBvQfrpTH6jYQd45Rzw3o=;\n\tb=syRh4W1tY+lnu38EqVjNxtkLZGHHMxP+U+m1JgISPun2MplnDVp2MGqcEQCJvVmmq/\n\tyB2FPWyxQQIrzmB1BXepBl8+P9bPW71MP8Vc6A7tsy2LQEgo3yNkanL3Wq6V14O+4yY8\n\t5CJgzL72j6xOFcL9DUAQwHua9lKL7H0aL/vQROeq5LR+YGza3xZDnM5NLdkGNeyJEQc7\n\t+kN/6bOoFYqP2htvfKtUkJJplu1ILk2GVVbr+v1biyQKPqFR5gilwrGmYOCCfiBTGp6g\n\tr54KKigdZnYaCzcA2sTl8RZUo3zpeb/dEchiMSk0XqCjRIixCGEujetc8HxvZ3nNqw+8\n\tylZA==","X-Gm-Message-State":"APjAAAUH1qx1JDWfEAlkZ5ZbqvSrh1S8zXOeCpCM5hyFpx0mae6ks4SE\n\tS2sWhPTzrbOY06tZmLDsNS2Nyyl8USE=","X-Google-Smtp-Source":"APXvYqxapvn6bo36wHh04Kqy9p7sTXH7wWnpwrYtlJAzhT17rGLXOlX9wRtaBDxdd0xZ6dqs56VIfQ==","X-Received":"by 2002:a2e:2b04:: with SMTP id q4mr6679288lje.175.1554463368883;\n\tFri, 05 Apr 2019 04:22:48 -0700 (PDT)","Date":"Fri, 5 Apr 2019 13:22:47 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405112247.GX23466@bigcity.dyn.berto.se>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-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":"<20190403150735.27580-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 05 Apr 2019 11:22:49 -0000"}},{"id":1284,"web_url":"https://patchwork.libcamera.org/comment/1284/","msgid":"<20190405154405.GA4636@pendragon.ideasonboard.com>","date":"2019-04-05T15:44:05","subject":"Re: [libcamera-devel] [PATCH v3 2/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 Wed, Apr 03, 2019 at 05:07:29PM +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, keep track of which streams have\n> been requested by the application, and configure 'output', 'viewfinder'\n> and 'stat' regardless of the user requests.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------\n> 1 file changed, 170 insertions(+), 46 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 164e187c769d..caf1051c58ab 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -171,17 +171,61 @@ private:\n> \t\tCIO2Device cio2_;\n> \t\tImgUDevice *imgu_;\n> \n> -\t\tStream stream_;\n> +\t\tStream outStream_;\n> +\t\tStream vfStream_;\n> +\n> +\t\tunsigned int activeStreamsMask;\n> \t};\n> \n> \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> \n> +\tstatic constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);\n> +\tstatic constexpr unsigned int IPU3_STREAM_VF = BIT(1);\n> +\n> \tIPU3CameraData *cameraData(const Camera *camera)\n> \t{\n> \t\treturn static_cast(\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> \n> +\tbool isOutput(IPU3CameraData *data, Stream *stream)\n> +\t{\n> +\t\treturn &data->outStream_ == stream;\n> +\t}\n\nYou're missing blank lines between functions.\n\n> +\tbool isOutputActive(IPU3CameraData *data)\n> +\t{\n> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?\n> +\t\t\ttrue : false;\n> +\t}\n> +\tvoid setOutputActive(IPU3CameraData *data)\n> +\t{\n> +\t\tdata->activeStreamsMask |= IPU3_STREAM_OUTPUT;\n> +\t}\n> +\tbool isViewfinder(IPU3CameraData *data, Stream *stream)\n> +\t{\n> +\t\treturn &data->vfStream_ == stream;\n> +\t}\n> +\tbool isViewfinderActive(IPU3CameraData *data)\n> +\t{\n> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_VF) ?\n> +\t\t\ttrue : false;\n> +\t}\n> +\tvoid setViewfinderActive(IPU3CameraData *data)\n> +\t{\n> +\t\tdata->activeStreamsMask |= IPU3_STREAM_VF;\n> +\t}\n> +\tbool isStreamActive(IPU3CameraData *data, Stream *stream)\n> +\t{\n> +\t\tif (isOutput(data, stream) &&\n> +\t\t isOutputActive(data))\n> +\t\t\treturn true;\n> +\t\tif (isViewfinder(data, stream) &&\n> +\t\t isViewfinderActive(data))\n> +\t\t\treturn true;\n\nHow about subclassing the Stream class and adding an active flag,\ninstead of storing that in a mask at the camera data level ? I think\nyou'll have more data to store in the IPU3CameraStream class.\n\n> +\n> +\t\treturn false;\n> +\t}\n> +\n> \tint registerCameras();\n> \n> \tImgUDevice imgu0_;\n> @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> {\n> \tstd::map 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> @@ -220,52 +264,93 @@ 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\nA bit inefficient as you end up copying the data twice, but probably\nnot the end of the world.\n\n> +\tLOG(IPU3, Debug)\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\nIf you also stored the stream name in the IPU3CameraStream (or\nIPU3Stream ?) class I think you could avoid code duplication.\n\n> \n> +\tconfigs[&data->vfStream_] = config;\n\nCan both streams scale ?\n\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 'viewfinder' 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> \treturn configs;\n> }\n> \n> int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> -\t\t\t\t\t std::map &config)\n> +\t\t\t\t\t std::map +\t\t\t\t\t\t StreamConfiguration> &config)\n\nThat doesn't look nice, I think you could keep the longer line instead.\n\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> +\tStreamConfiguration CIO2Config = {};\n\nVariables should start with lower case.\n\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> +\t/* Remove previously configured stream masks to store the new ones. */\n> +\tdata->activeStreamsMask = 0;\n> +\tfor (auto const &streamConfig : config) {\n> +\t\tStream *stream = streamConfig.first;\n> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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\nWhile at it, s/consider/Consider/ and s/32\"/32\"./\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<< \"Requested image format \" << cfg.width << \"x\"\n> +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> +\t\t\t<< camera->name() << \"'\";\n\nThat will be a bit confusing if you don't print the stream name, again,\nwith a Stream subclass, you could easily get the name :-)\n\n> +\n> +\t\t/*\n> +\t\t * FIXME: As viewfinder should be operated even when\n> +\t\t * applications do not intend to use it, we need to keep track\n> +\t\t * of which streams have to be configured, to make meaningful\n> +\t\t * decisions at configure and request queueing time.\n> +\t\t *\n> +\t\t * Walk here all the streams to configure and collect the\n> +\t\t * active ones in a bitmaks.\n> +\t\t */\n> +\t\tif (isOutput(data, stream))\n> +\t\t\tsetOutputActive(data);\n> +\t\tif (isViewfinder(data, stream))\n> +\t\t\tsetViewfinderActive(data);\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\nSounds good, but it's a bit strange to store that in a\nStreamConfiguration, given that you only use it to configure the CIO2\nand the ImgU input. How about replacing that by Size ?\n\n> \t}\n> \n> \t/*\n> @@ -281,26 +366,62 @@ 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> +\tfor (auto const &streamConfig : config) {\n> +\t\tStream *stream = streamConfig.first;\n> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> \n> -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\t\tif (isOutput(data, stream)) {\n> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> \n> -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\n> +\t\t\tif (isViewfinderActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Configure viewfinder using the output stream\n> +\t\t\t * configuration if it is not part of the active\n> +\t\t\t * streams list.\n> +\t\t\t */\n> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t} else if (isViewfinder(data, stream)) {\n> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tif (isOutputActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Configure output using the viewfinder stream\n> +\t\t\t * configuration if it is not part of the active\n> +\t\t\t * streams list.\n> +\t\t\t */\n> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\t}\n\nThis looks quite complicated. I think you can configure the streams\nincluded in the configuration in the loop without caring about their\ntype, and then configure the streams that are not included outside of\nthe loop. The code would look simpler.\n\n> \n> \treturn 0;\n> }\n> @@ -404,7 +525,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> +\tStream *stream = &data->outStream_;\n> \n> \t/* Queue a buffer to the ImgU output for capture. */\n> \tBuffer *buffer = request->findBuffer(stream);\n> @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> \t\tstd::unique_ptr data =\n> \t\t\tutils::make_unique(this);\n> -\t\tstd::set streams{ &data->stream_ };\n> +\t\tstd::set 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);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6CE760DB3\n\tfor ;\n\tFri, 5 Apr 2019 17:44:23 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.140.214.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 08E44E2;\n\tFri, 5 Apr 2019 17:44:19 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554479060;\n\tbh=3nFO5C9fRfLRFVhH0Sxizsgh2e3YxzXiwzx+eRyLF7Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=atQrChCJHluqYnsMYdUNioES6ZKrfxNAdblo4xnigIX66UNOCN5RNFw3TEoN80m4y\n\tkyqGmpWkpZzclEpz0XauaY4lbfu37cwWcIjUfxSlJDP0h1HcF6HC3/uhe638MVYout\n\t8TSmeEjGqdaKKmZYsPlwubEx/qt701IhnKeQmP84=","Date":"Fri, 5 Apr 2019 18:44:05 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405154405.GA4636@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190403150735.27580-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 05 Apr 2019 15:44:23 -0000"}},{"id":1297,"web_url":"https://patchwork.libcamera.org/comment/1297/","msgid":"<20190405234022.GD15350@bigcity.dyn.berto.se>","date":"2019-04-05T23:40:22","subject":"Re: [libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera\n\twith 2 streams","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi (again) Jacopo,\n\nOn 2019-04-05 13:22:47 +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n> \n> Thanks for your patch.\n> \n> On 2019-04-03 17:07:29 +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, keep track of which streams have\n> > been requested by the application, and configure 'output', 'viewfinder'\n> > and 'stat' regardless of the user requests.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------\n> > 1 file changed, 170 insertions(+), 46 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 164e187c769d..caf1051c58ab 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -171,17 +171,61 @@ private:\n> > \t\tCIO2Device cio2_;\n> > \t\tImgUDevice *imgu_;\n> > \n> > -\t\tStream stream_;\n> > +\t\tStream outStream_;\n> > +\t\tStream vfStream_;\n> > +\n> > +\t\tunsigned int activeStreamsMask;\n> > \t};\n> > \n> > \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > \n> > +\tstatic constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);\n> > +\tstatic constexpr unsigned int IPU3_STREAM_VF = BIT(1);\n> > +\n> > \tIPU3CameraData *cameraData(const Camera *camera)\n> > \t{\n> > \t\treturn static_cast(\n> > \t\t\tPipelineHandler::cameraData(camera));\n> > \t}\n> > \n> > +\tbool isOutput(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\treturn &data->outStream_ == stream;\n> > +\t}\n> > +\tbool isOutputActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\treturn (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?\n> > +\t\t\ttrue : false;\n> > +\t}\n> > +\tvoid setOutputActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\tdata->activeStreamsMask |= IPU3_STREAM_OUTPUT;\n> > +\t}\n> > +\tbool isViewfinder(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\treturn &data->vfStream_ == stream;\n> > +\t}\n> > +\tbool isViewfinderActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\treturn (data->activeStreamsMask & IPU3_STREAM_VF) ?\n> > +\t\t\ttrue : false;\n> > +\t}\n> > +\tvoid setViewfinderActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\tdata->activeStreamsMask |= IPU3_STREAM_VF;\n> > +\t}\n> > +\tbool isStreamActive(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\tif (isOutput(data, stream) &&\n> > +\t\t isOutputActive(data))\n> > +\t\t\treturn true;\n> > +\t\tif (isViewfinder(data, stream) &&\n> > +\t\t isViewfinderActive(data))\n> > +\t\t\treturn true;\n> > +\n> > +\t\treturn false;\n> > +\t}\n> > +\n> \n> Nit picking, I find the lack of new lines between functions above hard \n> to read :-) Also maybe you should add this new bit field and the helper \n> functions in a separate patch.\n> \n> > \tint registerCameras();\n> > \n> > \tImgUDevice imgu0_;\n> > @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > {\n> > \tstd::map 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> > @@ -220,52 +264,93 @@ 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 '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 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 'viewfinder' 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> This is not correct as streamConfiguration() would always return a map \n> with 2 elements even if only one stream is requested. Looking at your \n> private branch the top commit which make use of the stream usages \n> (stream roles in your case is it's based on the RFC) solves this in a \n> correct way. Maybe you should fold that change in here and depend on the \n> stream usage serries?\n\nI just took the for mentioned patch of your for a test drive. Good news \nit work with small changes to incorporate the latest CameraConfiguration \nchanges, really nice work extending the IPU3 pipeline handler to support \nmultiple streams!\n\nWhile doing so I noticed something you might want to consider when \nfolding it into this series. The top patch in your private branch do not \nallow for multiple streams to have the same usage role, it's locked to \none still image and one viewfinder.\n\n $ cam --camera \"ov5670 4-0036 1\" --capture --file \\\n --stream role=still,width=2560,height=1920 \\\n --stream role=viewfinder,width=640,height=480\n\nWorks as expected and frames form both streams are written to disc.\n\n $ cam --camera \"ov5670 4-0036 1\" --capture --file \\\n --stream role=still,width=2560,height=1920 \\\n --stream role=still,width=640,height=480\n\nDo not work as the IPU3 pipeline handler do not allow for two still \nimage streams. And the video usage role is not supported at all.\n\nIn my mental model of how this should work is that any combinations of \nstream usage roles should be supported by all pipeline handlers. The \nstream usage roles are used as 'hints' to the pipeline handlers to allow \nit to make better selection of which of its hardware pipelines to assign \nto each stream.\n\nMaybe my mental model is wrong and your interpretation is the correct \none. Maybe you share my model but as this patch is prototype as it's not \nyet posted to the ML. In any case I thought it best to bring it up so we \ncan align our views and avoid unnecessary work :-)\n\n> \n> > \n> > \treturn configs;\n> > }\n> > \n> > int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > -\t\t\t\t\t std::map &config)\n> > +\t\t\t\t\t std::map > +\t\t\t\t\t\t StreamConfiguration> &config)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> > +\tStreamConfiguration CIO2Config = {};\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> > +\t/* Remove previously configured stream masks to store the new ones. */\n> > +\tdata->activeStreamsMask = 0;\n> > +\tfor (auto const &streamConfig : config) {\n> > +\t\tStream *stream = streamConfig.first;\n> > +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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> > +\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<< \"Requested image format \" << cfg.width << \"x\"\n> > +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> > +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> > +\t\t\t<< camera->name() << \"'\";\n> > +\n> > +\t\t/*\n> > +\t\t * FIXME: As viewfinder should be operated even when\n> > +\t\t * applications do not intend to use it, we need to keep track\n> > +\t\t * of which streams have to be configured, to make meaningful\n> > +\t\t * decisions at configure and request queueing time.\n> > +\t\t *\n> > +\t\t * Walk here all the streams to configure and collect the\n> > +\t\t * active ones in a bitmaks.\n> > +\t\t */\n> > +\t\tif (isOutput(data, stream))\n> > +\t\t\tsetOutputActive(data);\n> > +\t\tif (isViewfinder(data, stream))\n> > +\t\t\tsetViewfinderActive(data);\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> > \t}\n> > \n> > \t/*\n> > @@ -281,26 +366,62 @@ 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> > +\tfor (auto const &streamConfig : config) {\n> > +\t\tStream *stream = streamConfig.first;\n> > +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> > \n> > -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\t\tif (isOutput(data, stream)) {\n> > +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > \n> > -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\n> > +\t\t\tif (isViewfinderActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Configure viewfinder using the output stream\n> > +\t\t\t * configuration if it is not part of the active\n> > +\t\t\t * streams list.\n> > +\t\t\t */\n> > +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t} else if (isViewfinder(data, stream)) {\n> > +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tif (isOutputActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Configure output using the viewfinder stream\n> > +\t\t\t * configuration if it is not part of the active\n> > +\t\t\t * streams list.\n> > +\t\t\t */\n> > +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n> \n> I see the need for the 'if (is*Active(data)) continue' logic but I \n> winder if you could not be a bit more clever about it :-)\n> \n> How about in the for (auto const &streamConfig : config) loop only \n> creating a cfg_output and cfg_viewfinder and outside the loop call \n> imgu->configureOutpu() once for each stream?\n> \n> > \n> > \treturn 0;\n> > }\n> > @@ -404,7 +525,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> > +\tStream *stream = &data->outStream_;\n> > \n> > \t/* Queue a buffer to the ImgU output for capture. */\n> > \tBuffer *buffer = request->findBuffer(stream);\n> > @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> > \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > \t\tstd::unique_ptr data =\n> > \t\t\tutils::make_unique(this);\n> > -\t\tstd::set streams{ &data->stream_ };\n> > +\t\tstd::set 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> > -- \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\n> \n> -- \n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08BBE610B3\n\tfor ;\n\tSat, 6 Apr 2019 01:40:25 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id a28so5526758lfo.7\n\tfor ;\n\tFri, 05 Apr 2019 16:40:24 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tt14sm5003676lji.33.2019.04.05.16.40.22\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 05 Apr 2019 16:40:23 -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=2UA61ic03vbXTtFVxzMOs8BGtfKlL1t8acEr1SyCJCI=;\n\tb=2H6UGAaltZDXQQOuDQfhAkOL/hwGHtPFt/CVFfJCOvRuQzh2enB6lyMWRw/Tu/qZWu\n\tfRRrMW7msNiHKHhJY2ENAFIImKERdKmyR59i+nEOtrA5GLCr4W2k4/WvRZ7x1h9H/evM\n\ttLlxIG658O8XLBvZXvGXDA8SgHtpEzLrk/33UahQoUAPzd8Q6SzIEyyRGalB5kKkG3OH\n\twVdUy2LRGMhPimTTxrPIWCZG4qtFjHh+1gT16nRCV8wY5EKz2K5cT0c8TBPs27Dr2xXY\n\tigoUJldCiO9EJgFFdqbAd+UdpDwJCBAIiVYrvJIFQkWqh40wP5PEERoL+geUcL48YWtl\n\tgLCg==","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=2UA61ic03vbXTtFVxzMOs8BGtfKlL1t8acEr1SyCJCI=;\n\tb=s1uRjLwwabSkhA7kSleMcO2i46VhjsHojuWGEKoIXJczEDTyo82IZMn5mDMhPE8jR5\n\tBBGHaSQsNdp6dcYNDyP5cwXbDpZLHkk3qCXqt7OOY2aBrKgQEWpBAHhJ/Y3TonPmjeWc\n\tYXjPrGCwG0mMpHzMkvHtBtAA8a2V9K+04KkyxPpT8u2bvPbiNkDxTaFNMGJrqk0rNQHR\n\tvkMOcTXmzIq32iLRQcf7LCiHk4xlm1iGX5Y8rMXglMDUCCxe7kXyhA6iw9DB8WD24gK+\n\tZr7A5SYJbtwrARo99aBfYmSoRQI56dhcuMDBBq/8XL0tm26bFzokrG3vmUXrq5jY9d9z\n\tFaEA==","X-Gm-Message-State":"APjAAAVdeqP/yVghkcV8jftaSDn/4x/iRDBReKtMYpaZZiFgQtOnJvpV\n\tfq1KHfPn8c36ooeIGG7CQCVX6Q==","X-Google-Smtp-Source":"APXvYqzJBIhtBYuMq0GwbIDBiZ5lsVr5eVxY4OTkEb3FhbtutAKWO3wrEK3cmMtks3+rynBZta3yhA==","X-Received":"by 2002:ac2:598b:: with SMTP id w11mr629191lfn.139.1554507624255;\n\tFri, 05 Apr 2019 16:40:24 -0700 (PDT)","Date":"Sat, 6 Apr 2019 01:40:22 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405234022.GD15350@bigcity.dyn.berto.se>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-jacopo@jmondi.org>\n\t<20190405112247.GX23466@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190405112247.GX23466@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 05 Apr 2019 23:40:25 -0000"}},{"id":1299,"web_url":"https://patchwork.libcamera.org/comment/1299/","msgid":"<20190406162453.GB4817@pendragon.ideasonboard.com>","date":"2019-04-06T16:24:53","subject":"Re: [libcamera-devel] [PATCH v3 2/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 Niklas,\n\nOn Sat, Apr 06, 2019 at 01:40:22AM +0200, Niklas Söderlund wrote:\n> On 2019-04-05 13:22:47 +0200, Niklas Söderlund wrote:\n> > On 2019-04-03 17:07:29 +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, keep track of which streams have\n> >> been requested by the application, and configure 'output', 'viewfinder'\n> >> and 'stat' regardless of the user requests.\n> >> \n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------\n> >> 1 file changed, 170 insertions(+), 46 deletions(-)\n> >> \n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 164e187c769d..caf1051c58ab 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -171,17 +171,61 @@ private:\n> >> \t\tCIO2Device cio2_;\n> >> \t\tImgUDevice *imgu_;\n> >> \n> >> -\t\tStream stream_;\n> >> +\t\tStream outStream_;\n> >> +\t\tStream vfStream_;\n> >> +\n> >> +\t\tunsigned int activeStreamsMask;\n> >> \t};\n> >> \n> >> \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> >> \n> >> +\tstatic constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);\n> >> +\tstatic constexpr unsigned int IPU3_STREAM_VF = BIT(1);\n> >> +\n> >> \tIPU3CameraData *cameraData(const Camera *camera)\n> >> \t{\n> >> \t\treturn static_cast(\n> >> \t\t\tPipelineHandler::cameraData(camera));\n> >> \t}\n> >> \n> >> +\tbool isOutput(IPU3CameraData *data, Stream *stream)\n> >> +\t{\n> >> +\t\treturn &data->outStream_ == stream;\n> >> +\t}\n> >> +\tbool isOutputActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?\n> >> +\t\t\ttrue : false;\n> >> +\t}\n> >> +\tvoid setOutputActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\tdata->activeStreamsMask |= IPU3_STREAM_OUTPUT;\n> >> +\t}\n> >> +\tbool isViewfinder(IPU3CameraData *data, Stream *stream)\n> >> +\t{\n> >> +\t\treturn &data->vfStream_ == stream;\n> >> +\t}\n> >> +\tbool isViewfinderActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_VF) ?\n> >> +\t\t\ttrue : false;\n> >> +\t}\n> >> +\tvoid setViewfinderActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\tdata->activeStreamsMask |= IPU3_STREAM_VF;\n> >> +\t}\n> >> +\tbool isStreamActive(IPU3CameraData *data, Stream *stream)\n> >> +\t{\n> >> +\t\tif (isOutput(data, stream) &&\n> >> +\t\t isOutputActive(data))\n> >> +\t\t\treturn true;\n> >> +\t\tif (isViewfinder(data, stream) &&\n> >> +\t\t isViewfinderActive(data))\n> >> +\t\t\treturn true;\n> >> +\n> >> +\t\treturn false;\n> >> +\t}\n> >> +\n> > \n> > Nit picking, I find the lack of new lines between functions above hard \n> > to read :-) Also maybe you should add this new bit field and the helper \n> > functions in a separate patch.\n> > \n> >> \tint registerCameras();\n> >> \n> >> \tImgUDevice imgu0_;\n> >> @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> >> {\n> >> \tstd::map 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> >> @@ -220,52 +264,93 @@ 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 '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 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 'viewfinder' 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> > This is not correct as streamConfiguration() would always return a map \n> > with 2 elements even if only one stream is requested. Looking at your \n> > private branch the top commit which make use of the stream usages \n> > (stream roles in your case is it's based on the RFC) solves this in a \n> > correct way. Maybe you should fold that change in here and depend on the \n> > stream usage serries?\n> \n> I just took the for mentioned patch of your for a test drive. Good news \n> it work with small changes to incorporate the latest CameraConfiguration \n> changes, really nice work extending the IPU3 pipeline handler to support \n> multiple streams!\n> \n> While doing so I noticed something you might want to consider when \n> folding it into this series. The top patch in your private branch do not \n> allow for multiple streams to have the same usage role, it's locked to \n> one still image and one viewfinder.\n> \n> $ cam --camera \"ov5670 4-0036 1\" --capture --file \\\n> --stream role=still,width=2560,height=1920 \\\n> --stream role=viewfinder,width=640,height=480\n> \n> Works as expected and frames form both streams are written to disc.\n> \n> $ cam --camera \"ov5670 4-0036 1\" --capture --file \\\n> --stream role=still,width=2560,height=1920 \\\n> --stream role=still,width=640,height=480\n> \n> Do not work as the IPU3 pipeline handler do not allow for two still \n> image streams. And the video usage role is not supported at all.\n> \n> In my mental model of how this should work is that any combinations of \n> stream usage roles should be supported by all pipeline handlers. The \n> stream usage roles are used as 'hints' to the pipeline handlers to allow \n> it to make better selection of which of its hardware pipelines to assign \n> to each stream.\n\nI agree overall, but \"any combination\" may be a bit too much. Pipeline\nhandlers should all combinations that are achievable from a hardware\npoint of view. For instance, if an application requests two viewfinder\nstreams, or one viewfinder and one video recording stream, while the\nhardware can produce two streams but only one of them can produce video,\nwhile the other is limited to still images with a max frame rate of one\nframe every 3 seconds, that request should likely fail.\n\n> Maybe my mental model is wrong and your interpretation is the correct \n> one. Maybe you share my model but as this patch is prototype as it's not \n> yet posted to the ML. In any case I thought it best to bring it up so we \n> can align our views and avoid unnecessary work :-)\n> \n> > \n> >> \n> >> \treturn configs;\n> >> }\n> >> \n> >> int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >> -\t\t\t\t\t std::map &config)\n> >> +\t\t\t\t\t std::map >> +\t\t\t\t\t\t StreamConfiguration> &config)\n> >> {\n> >> \tIPU3CameraData *data = cameraData(camera);\n> >> -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> >> +\tStreamConfiguration CIO2Config = {};\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> >> +\t/* Remove previously configured stream masks to store the new ones. */\n> >> +\tdata->activeStreamsMask = 0;\n> >> +\tfor (auto const &streamConfig : config) {\n> >> +\t\tStream *stream = streamConfig.first;\n> >> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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> >> +\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<< \"Requested image format \" << cfg.width << \"x\"\n> >> +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> >> +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> >> +\t\t\t<< camera->name() << \"'\";\n> >> +\n> >> +\t\t/*\n> >> +\t\t * FIXME: As viewfinder should be operated even when\n> >> +\t\t * applications do not intend to use it, we need to keep track\n> >> +\t\t * of which streams have to be configured, to make meaningful\n> >> +\t\t * decisions at configure and request queueing time.\n> >> +\t\t *\n> >> +\t\t * Walk here all the streams to configure and collect the\n> >> +\t\t * active ones in a bitmaks.\n> >> +\t\t */\n> >> +\t\tif (isOutput(data, stream))\n> >> +\t\t\tsetOutputActive(data);\n> >> +\t\tif (isViewfinder(data, stream))\n> >> +\t\t\tsetViewfinderActive(data);\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> >> \t}\n> >> \n> >> \t/*\n> >> @@ -281,26 +366,62 @@ 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> >> +\tfor (auto const &streamConfig : config) {\n> >> +\t\tStream *stream = streamConfig.first;\n> >> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> >> \n> >> -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\t\tif (isOutput(data, stream)) {\n> >> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> \n> >> -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\n> >> +\t\t\tif (isViewfinderActive(data))\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\t/*\n> >> +\t\t\t * Configure viewfinder using the output stream\n> >> +\t\t\t * configuration if it is not part of the active\n> >> +\t\t\t * streams list.\n> >> +\t\t\t */\n> >> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\t\t} else if (isViewfinder(data, stream)) {\n> >> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\t\t\tif (isOutputActive(data))\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\t/*\n> >> +\t\t\t * Configure output using the viewfinder stream\n> >> +\t\t\t * configuration if it is not part of the active\n> >> +\t\t\t * streams list.\n> >> +\t\t\t */\n> >> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\t\t}\n> >> +\t}\n> > \n> > I see the need for the 'if (is*Active(data)) continue' logic but I \n> > winder if you could not be a bit more clever about it :-)\n> > \n> > How about in the for (auto const &streamConfig : config) loop only \n> > creating a cfg_output and cfg_viewfinder and outside the loop call \n> > imgu->configureOutpu() once for each stream?\n> > \n> >> \n> >> \treturn 0;\n> >> }\n> >> @@ -404,7 +525,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> >> +\tStream *stream = &data->outStream_;\n> >> \n> >> \t/* Queue a buffer to the ImgU output for capture. */\n> >> \tBuffer *buffer = request->findBuffer(stream);\n> >> @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> >> \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> >> \t\tstd::unique_ptr data =\n> >> \t\t\tutils::make_unique(this);\n> >> -\t\tstd::set streams{ &data->stream_ };\n> >> +\t\tstd::set 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);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8DDF260B1B\n\tfor ;\n\tSat, 6 Apr 2019 18:25:08 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [91.183.39.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09FA399F;\n\tSat, 6 Apr 2019 18:25:07 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554567908;\n\tbh=+i7xTrapTJmnwh6mgflvVfHGg2oarjzFXHTdjrZJT34=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W5J7OdSeQhR8n9+n0nS3x/288lbx2ov94SobR8XzLvkK4TfX94te6NQ38tnYWiX9D\n\tkoAvKh5+tlLrY/ZFWa2+JghxAEETeeMFpZzXplQf1/xS54vsiGDEWkZ8Kbiw6Hq1Hz\n\tbnvC8hXo2+SqE1XwVPqrMtXJxs3NLLrsh0e4JcTc=","Date":"Sat, 6 Apr 2019 19:24:53 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190406162453.GB4817@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-jacopo@jmondi.org>\n\t<20190405112247.GX23466@bigcity.dyn.berto.se>\n\t<20190405234022.GD15350@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190405234022.GD15350@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 06 Apr 2019 16:25:08 -0000"}},{"id":1306,"web_url":"https://patchwork.libcamera.org/comment/1306/","msgid":"<20190408075408.ibf4rqpkcudmo32z@uno.localdomain>","date":"2019-04-08T07:54:08","subject":"Re: [libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera\n\twith 2 streams","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for review,\n\nOn Fri, Apr 05, 2019 at 06:44:05PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 03, 2019 at 05:07:29PM +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, keep track of which streams have\n> > been requested by the application, and configure 'output', 'viewfinder'\n> > and 'stat' regardless of the user requests.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------\n> > 1 file changed, 170 insertions(+), 46 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 164e187c769d..caf1051c58ab 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -171,17 +171,61 @@ private:\n> > \t\tCIO2Device cio2_;\n> > \t\tImgUDevice *imgu_;\n> >\n> > -\t\tStream stream_;\n> > +\t\tStream outStream_;\n> > +\t\tStream vfStream_;\n> > +\n> > +\t\tunsigned int activeStreamsMask;\n> > \t};\n> >\n> > \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> >\n> > +\tstatic constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);\n> > +\tstatic constexpr unsigned int IPU3_STREAM_VF = BIT(1);\n> > +\n> > \tIPU3CameraData *cameraData(const Camera *camera)\n> > \t{\n> > \t\treturn static_cast(\n> > \t\t\tPipelineHandler::cameraData(camera));\n> > \t}\n> >\n> > +\tbool isOutput(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\treturn &data->outStream_ == stream;\n> > +\t}\n>\n> You're missing blank lines between functions.\n>\n\nThat was a style choice, to make those functions look more 'compact'\ntogether. But since both you and Niklas pointed this out, I'll change\nit..\n\n> > +\tbool isOutputActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\treturn (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?\n> > +\t\t\ttrue : false;\n> > +\t}\n> > +\tvoid setOutputActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\tdata->activeStreamsMask |= IPU3_STREAM_OUTPUT;\n> > +\t}\n> > +\tbool isViewfinder(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\treturn &data->vfStream_ == stream;\n> > +\t}\n> > +\tbool isViewfinderActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\treturn (data->activeStreamsMask & IPU3_STREAM_VF) ?\n> > +\t\t\ttrue : false;\n> > +\t}\n> > +\tvoid setViewfinderActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\tdata->activeStreamsMask |= IPU3_STREAM_VF;\n> > +\t}\n> > +\tbool isStreamActive(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\tif (isOutput(data, stream) &&\n> > +\t\t isOutputActive(data))\n> > +\t\t\treturn true;\n> > +\t\tif (isViewfinder(data, stream) &&\n> > +\t\t isViewfinderActive(data))\n> > +\t\t\treturn true;\n>\n> How about subclassing the Stream class and adding an active flag,\n> instead of storing that in a mask at the camera data level ? I think\n> you'll have more data to store in the IPU3CameraStream class.\n>\n\nBy sub-classing Stream I can collect per-Stream informations, like an\n'active' flag, what I need here is a global status that makes possible\nto do choices like \"should I configure viewfinder now that I'm\nconfiguring output too, as viewfinder is not part of the active stream\nlist?\"\n\nTrue I can store a per-stream flag and check for that in each stream\nthe pipeline handler supports. I'll see how many other things could\nfit in a Stream sub-class and consider this option, thanks!\n\n> > +\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > \tint registerCameras();\n> >\n> > \tImgUDevice imgu0_;\n> > @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > {\n> > \tstd::map 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> > @@ -220,52 +264,93 @@ 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>\n> A bit inefficient as you end up copying the data twice, but probably\n> not the end of the world.\n>\n> > +\tLOG(IPU3, Debug)\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> If you also stored the stream name in the IPU3CameraStream (or\n> IPU3Stream ?) class I think you could avoid code duplication.\n>\n> >\n> > +\tconfigs[&data->vfStream_] = config;\n>\n> Can both streams scale ?\n>\n\nCan't they? Why is it relevant here?\n\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 'viewfinder' 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> > \treturn configs;\n> > }\n> >\n> > int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > -\t\t\t\t\t std::map &config)\n> > +\t\t\t\t\t std::map > +\t\t\t\t\t\t StreamConfiguration> &config)\n>\n> That doesn't look nice, I think you could keep the longer line instead.\n>\n\nI like 80-cols :(\n\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> > +\tStreamConfiguration CIO2Config = {};\n>\n> Variables should start with lower case.\n>\n\nAck.. cio2Config then\n\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> > +\t/* Remove previously configured stream masks to store the new ones. */\n> > +\tdata->activeStreamsMask = 0;\n> > +\tfor (auto const &streamConfig : config) {\n> > +\t\tStream *stream = streamConfig.first;\n> > +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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>\n> While at it, s/consider/Consider/ and s/32\"/32\"./\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<< \"Requested image format \" << cfg.width << \"x\"\n> > +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> > +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> > +\t\t\t<< camera->name() << \"'\";\n>\n> That will be a bit confusing if you don't print the stream name, again,\n> with a Stream subclass, you could easily get the name :-)\n>\n> > +\n> > +\t\t/*\n> > +\t\t * FIXME: As viewfinder should be operated even when\n> > +\t\t * applications do not intend to use it, we need to keep track\n> > +\t\t * of which streams have to be configured, to make meaningful\n> > +\t\t * decisions at configure and request queueing time.\n> > +\t\t *\n> > +\t\t * Walk here all the streams to configure and collect the\n> > +\t\t * active ones in a bitmaks.\n> > +\t\t */\n> > +\t\tif (isOutput(data, stream))\n> > +\t\t\tsetOutputActive(data);\n> > +\t\tif (isViewfinder(data, stream))\n> > +\t\t\tsetViewfinderActive(data);\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> Sounds good, but it's a bit strange to store that in a\n> StreamConfiguration, given that you only use it to configure the CIO2\n> and the ImgU input. How about replacing that by Size ?\n>\n\nThat's also a possibility, I only use the width and height (I now\nwonder what should I do with the configuration's pixelformat fields though)\n\n> > \t}\n> >\n> > \t/*\n> > @@ -281,26 +366,62 @@ 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> > +\tfor (auto const &streamConfig : config) {\n> > +\t\tStream *stream = streamConfig.first;\n> > +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> >\n> > -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\t\tif (isOutput(data, stream)) {\n> > +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> >\n> > -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\n> > +\t\t\tif (isViewfinderActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Configure viewfinder using the output stream\n> > +\t\t\t * configuration if it is not part of the active\n> > +\t\t\t * streams list.\n> > +\t\t\t */\n> > +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t} else if (isViewfinder(data, stream)) {\n> > +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tif (isOutputActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Configure output using the viewfinder stream\n> > +\t\t\t * configuration if it is not part of the active\n> > +\t\t\t * streams list.\n> > +\t\t\t */\n> > +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n>\n> This looks quite complicated. I think you can configure the streams\n> included in the configuration in the loop without caring about their\n> type, and then configure the streams that are not included outside of\n> the loop. The code would look simpler.\n>\n\nNiklas had a similar idea, I'll try to see how it looks like.\n\nThanks\n j\n> >\n> > \treturn 0;\n> > }\n> > @@ -404,7 +525,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> > +\tStream *stream = &data->outStream_;\n> >\n> > \t/* Queue a buffer to the ImgU output for capture. */\n> > \tBuffer *buffer = request->findBuffer(stream);\n> > @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> > \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > \t\tstd::unique_ptr data =\n> > \t\t\tutils::make_unique(this);\n> > -\t\tstd::set streams{ &data->stream_ };\n> > +\t\tstd::set 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>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7E3560B23\n\tfor ;\n\tMon, 8 Apr 2019 09:53:20 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 230771C000E;\n\tMon, 8 Apr 2019 07:53:19 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Apr 2019 09:54:08 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408075408.ibf4rqpkcudmo32z@uno.localdomain>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-jacopo@jmondi.org>\n\t<20190405154405.GA4636@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"evbc3j75ebo5nffo\"","Content-Disposition":"inline","In-Reply-To":"<20190405154405.GA4636@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 08 Apr 2019 07:53:20 -0000"}},{"id":1307,"web_url":"https://patchwork.libcamera.org/comment/1307/","msgid":"<20190408075815.2bb3of4hx4tencal@uno.localdomain>","date":"2019-04-08T07:58:15","subject":"Re: [libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera\n\twith 2 streams","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Apr 05, 2019 at 01:22:47PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2019-04-03 17:07:29 +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, keep track of which streams have\n> > been requested by the application, and configure 'output', 'viewfinder'\n> > and 'stat' regardless of the user requests.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------\n> > 1 file changed, 170 insertions(+), 46 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 164e187c769d..caf1051c58ab 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -171,17 +171,61 @@ private:\n> > \t\tCIO2Device cio2_;\n> > \t\tImgUDevice *imgu_;\n> >\n> > -\t\tStream stream_;\n> > +\t\tStream outStream_;\n> > +\t\tStream vfStream_;\n> > +\n> > +\t\tunsigned int activeStreamsMask;\n> > \t};\n> >\n> > \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> >\n> > +\tstatic constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);\n> > +\tstatic constexpr unsigned int IPU3_STREAM_VF = BIT(1);\n> > +\n> > \tIPU3CameraData *cameraData(const Camera *camera)\n> > \t{\n> > \t\treturn static_cast(\n> > \t\t\tPipelineHandler::cameraData(camera));\n> > \t}\n> >\n> > +\tbool isOutput(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\treturn &data->outStream_ == stream;\n> > +\t}\n> > +\tbool isOutputActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\treturn (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?\n> > +\t\t\ttrue : false;\n> > +\t}\n> > +\tvoid setOutputActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\tdata->activeStreamsMask |= IPU3_STREAM_OUTPUT;\n> > +\t}\n> > +\tbool isViewfinder(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\treturn &data->vfStream_ == stream;\n> > +\t}\n> > +\tbool isViewfinderActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\treturn (data->activeStreamsMask & IPU3_STREAM_VF) ?\n> > +\t\t\ttrue : false;\n> > +\t}\n> > +\tvoid setViewfinderActive(IPU3CameraData *data)\n> > +\t{\n> > +\t\tdata->activeStreamsMask |= IPU3_STREAM_VF;\n> > +\t}\n> > +\tbool isStreamActive(IPU3CameraData *data, Stream *stream)\n> > +\t{\n> > +\t\tif (isOutput(data, stream) &&\n> > +\t\t isOutputActive(data))\n> > +\t\t\treturn true;\n> > +\t\tif (isViewfinder(data, stream) &&\n> > +\t\t isViewfinderActive(data))\n> > +\t\t\treturn true;\n> > +\n> > +\t\treturn false;\n> > +\t}\n> > +\n>\n> Nit picking, I find the lack of new lines between functions above hard\n> to read :-) Also maybe you should add this new bit field and the helper\n> functions in a separate patch.\n>\n\nLaurent had the same comment, I'll break them.\n\nOn the separate patch, as you seem to ask it quite often, I introduced\nthem here as they are used here... Introducing them separately without\nusers makes it hard for reviewer to see how they are actually used\n(well, here it is quite trivial :)\n\nI understand smaller patches are less tiring to review, but sometimes\ndemand the reviewer to jump from one patch to the other, which I\nreally hate, personally :)\n\nI guess it's a matter of tastes...\n\nThanks\n j\n\n> > \tint registerCameras();\n> >\n> > \tImgUDevice imgu0_;\n> > @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > {\n> > \tstd::map 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> > @@ -220,52 +264,93 @@ 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 '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 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 'viewfinder' 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> This is not correct as streamConfiguration() would always return a map\n> with 2 elements even if only one stream is requested. Looking at your\n> private branch the top commit which make use of the stream usages\n> (stream roles in your case is it's based on the RFC) solves this in a\n> correct way. Maybe you should fold that change in here and depend on the\n> stream usage serries?\n>\n> >\n> > \treturn configs;\n> > }\n> >\n> > int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > -\t\t\t\t\t std::map &config)\n> > +\t\t\t\t\t std::map > +\t\t\t\t\t\t StreamConfiguration> &config)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> > +\tStreamConfiguration CIO2Config = {};\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> > +\t/* Remove previously configured stream masks to store the new ones. */\n> > +\tdata->activeStreamsMask = 0;\n> > +\tfor (auto const &streamConfig : config) {\n> > +\t\tStream *stream = streamConfig.first;\n> > +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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> > +\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<< \"Requested image format \" << cfg.width << \"x\"\n> > +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> > +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> > +\t\t\t<< camera->name() << \"'\";\n> > +\n> > +\t\t/*\n> > +\t\t * FIXME: As viewfinder should be operated even when\n> > +\t\t * applications do not intend to use it, we need to keep track\n> > +\t\t * of which streams have to be configured, to make meaningful\n> > +\t\t * decisions at configure and request queueing time.\n> > +\t\t *\n> > +\t\t * Walk here all the streams to configure and collect the\n> > +\t\t * active ones in a bitmaks.\n> > +\t\t */\n> > +\t\tif (isOutput(data, stream))\n> > +\t\t\tsetOutputActive(data);\n> > +\t\tif (isViewfinder(data, stream))\n> > +\t\t\tsetViewfinderActive(data);\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> > \t}\n> >\n> > \t/*\n> > @@ -281,26 +366,62 @@ 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> > +\tfor (auto const &streamConfig : config) {\n> > +\t\tStream *stream = streamConfig.first;\n> > +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> >\n> > -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\t\tif (isOutput(data, stream)) {\n> > +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> >\n> > -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\n> > +\t\t\tif (isViewfinderActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Configure viewfinder using the output stream\n> > +\t\t\t * configuration if it is not part of the active\n> > +\t\t\t * streams list.\n> > +\t\t\t */\n> > +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t} else if (isViewfinder(data, stream)) {\n> > +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tif (isOutputActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Configure output using the viewfinder stream\n> > +\t\t\t * configuration if it is not part of the active\n> > +\t\t\t * streams list.\n> > +\t\t\t */\n> > +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n>\n> I see the need for the 'if (is*Active(data)) continue' logic but I\n> winder if you could not be a bit more clever about it :-)\n>\n> How about in the for (auto const &streamConfig : config) loop only\n> creating a cfg_output and cfg_viewfinder and outside the loop call\n> imgu->configureOutpu() once for each stream?\n>\n> >\n> > \treturn 0;\n> > }\n> > @@ -404,7 +525,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> > +\tStream *stream = &data->outStream_;\n> >\n> > \t/* Queue a buffer to the ImgU output for capture. */\n> > \tBuffer *buffer = request->findBuffer(stream);\n> > @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> > \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > \t\tstd::unique_ptr data =\n> > \t\t\tutils::make_unique(this);\n> > -\t\tstd::set streams{ &data->stream_ };\n> > +\t\tstd::set 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> > --\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\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 56D5A60B23\n\tfor ;\n\tMon, 8 Apr 2019 09:57:27 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id AD8BC40003;\n\tMon, 8 Apr 2019 07:57:26 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Apr 2019 09:58:15 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408075815.2bb3of4hx4tencal@uno.localdomain>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-jacopo@jmondi.org>\n\t<20190405112247.GX23466@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ta4w3uzd6qhimfuu\"","Content-Disposition":"inline","In-Reply-To":"<20190405112247.GX23466@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 08 Apr 2019 07:57:27 -0000"}},{"id":1313,"web_url":"https://patchwork.libcamera.org/comment/1313/","msgid":"<20190408130256.GC4888@pendragon.ideasonboard.com>","date":"2019-04-08T13:02:56","subject":"Re: [libcamera-devel] [PATCH v3 2/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.\n\nOn Mon, Apr 08, 2019 at 09:54:08AM +0200, Jacopo Mondi wrote:\n> On Fri, Apr 05, 2019 at 06:44:05PM +0300, Laurent Pinchart wrote:\n> > On Wed, Apr 03, 2019 at 05:07:29PM +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, keep track of which streams have\n> >> been requested by the application, and configure 'output', 'viewfinder'\n> >> and 'stat' regardless of the user requests.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------\n> >> 1 file changed, 170 insertions(+), 46 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 164e187c769d..caf1051c58ab 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -171,17 +171,61 @@ private:\n> >> \t\tCIO2Device cio2_;\n> >> \t\tImgUDevice *imgu_;\n> >>\n> >> -\t\tStream stream_;\n> >> +\t\tStream outStream_;\n> >> +\t\tStream vfStream_;\n> >> +\n> >> +\t\tunsigned int activeStreamsMask;\n> >> \t};\n> >>\n> >> \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> >>\n> >> +\tstatic constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);\n> >> +\tstatic constexpr unsigned int IPU3_STREAM_VF = BIT(1);\n> >> +\n> >> \tIPU3CameraData *cameraData(const Camera *camera)\n> >> \t{\n> >> \t\treturn static_cast(\n> >> \t\t\tPipelineHandler::cameraData(camera));\n> >> \t}\n> >>\n> >> +\tbool isOutput(IPU3CameraData *data, Stream *stream)\n> >> +\t{\n> >> +\t\treturn &data->outStream_ == stream;\n> >> +\t}\n> >\n> > You're missing blank lines between functions.\n> >\n> \n> That was a style choice, to make those functions look more 'compact'\n> together. But since both you and Niklas pointed this out, I'll change\n> it..\n> \n> >> +\tbool isOutputActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?\n> >> +\t\t\ttrue : false;\n> >> +\t}\n> >> +\tvoid setOutputActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\tdata->activeStreamsMask |= IPU3_STREAM_OUTPUT;\n> >> +\t}\n> >> +\tbool isViewfinder(IPU3CameraData *data, Stream *stream)\n> >> +\t{\n> >> +\t\treturn &data->vfStream_ == stream;\n> >> +\t}\n> >> +\tbool isViewfinderActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\treturn (data->activeStreamsMask & IPU3_STREAM_VF) ?\n> >> +\t\t\ttrue : false;\n> >> +\t}\n> >> +\tvoid setViewfinderActive(IPU3CameraData *data)\n> >> +\t{\n> >> +\t\tdata->activeStreamsMask |= IPU3_STREAM_VF;\n> >> +\t}\n> >> +\tbool isStreamActive(IPU3CameraData *data, Stream *stream)\n> >> +\t{\n> >> +\t\tif (isOutput(data, stream) &&\n> >> +\t\t isOutputActive(data))\n> >> +\t\t\treturn true;\n> >> +\t\tif (isViewfinder(data, stream) &&\n> >> +\t\t isViewfinderActive(data))\n> >> +\t\t\treturn true;\n> >\n> > How about subclassing the Stream class and adding an active flag,\n> > instead of storing that in a mask at the camera data level ? I think\n> > you'll have more data to store in the IPU3CameraStream class.\n> \n> By sub-classing Stream I can collect per-Stream informations, like an\n> 'active' flag, what I need here is a global status that makes possible\n> to do choices like \"should I configure viewfinder now that I'm\n> configuring output too, as viewfinder is not part of the active stream\n> list?\"\n\nI agree that a global bitmask will also be needed, but you may be able\nto turn it into local variables if you have a per-stream active state.\n\n> True I can store a per-stream flag and check for that in each stream\n> the pipeline handler supports. I'll see how many other things could\n> fit in a Stream sub-class and consider this option, thanks!\n\nExactly :-)\n\n> >> +\n> >> +\t\treturn false;\n> >> +\t}\n> >> +\n> >> \tint registerCameras();\n> >>\n> >> \tImgUDevice imgu0_;\n> >> @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> >> {\n> >> \tstd::map 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> >> @@ -220,52 +264,93 @@ 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> >\n> > A bit inefficient as you end up copying the data twice, but probably\n> > not the end of the world.\n> >\n> >> +\tLOG(IPU3, Debug)\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> > If you also stored the stream name in the IPU3CameraStream (or\n> > IPU3Stream ?) class I think you could avoid code duplication.\n> >\n> >> +\tconfigs[&data->vfStream_] = config;\n> >\n> > Can both streams scale ?\n> \n> Can't they?\n\nI don't know, it was a true question :-)\n\n> Why is it relevant here?\n\nBecause you're configuring both streams with the same output size,\ndifferent than the sensor size, so you need the scale on both streams.\n\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 'viewfinder' 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> >> \treturn configs;\n> >> }\n> >>\n> >> int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >> -\t\t\t\t\t std::map &config)\n> >> +\t\t\t\t\t std::map >> +\t\t\t\t\t\t StreamConfiguration> &config)\n> >\n> > That doesn't look nice, I think you could keep the longer line instead.\n> \n> I like 80-cols :(\n\nSo do I, but something small exceptions are useful too :-)\n\n> >> {\n> >> \tIPU3CameraData *data = cameraData(camera);\n> >> -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> >> +\tStreamConfiguration CIO2Config = {};\n> >\n> > Variables should start with lower case.\n> \n> Ack.. cio2Config then\n> \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> >> +\t/* Remove previously configured stream masks to store the new ones. */\n> >> +\tdata->activeStreamsMask = 0;\n> >> +\tfor (auto const &streamConfig : config) {\n> >> +\t\tStream *stream = streamConfig.first;\n> >> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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> >\n> > While at it, s/consider/Consider/ and s/32\"/32\"./\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<< \"Requested image format \" << cfg.width << \"x\"\n> >> +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> >> +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> >> +\t\t\t<< camera->name() << \"'\";\n> >\n> > That will be a bit confusing if you don't print the stream name, again,\n> > with a Stream subclass, you could easily get the name :-)\n> >\n> >> +\n> >> +\t\t/*\n> >> +\t\t * FIXME: As viewfinder should be operated even when\n> >> +\t\t * applications do not intend to use it, we need to keep track\n> >> +\t\t * of which streams have to be configured, to make meaningful\n> >> +\t\t * decisions at configure and request queueing time.\n> >> +\t\t *\n> >> +\t\t * Walk here all the streams to configure and collect the\n> >> +\t\t * active ones in a bitmaks.\n> >> +\t\t */\n> >> +\t\tif (isOutput(data, stream))\n> >> +\t\t\tsetOutputActive(data);\n> >> +\t\tif (isViewfinder(data, stream))\n> >> +\t\t\tsetViewfinderActive(data);\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> > Sounds good, but it's a bit strange to store that in a\n> > StreamConfiguration, given that you only use it to configure the CIO2\n> > and the ImgU input. How about replacing that by Size ?\n> \n> That's also a possibility, I only use the width and height (I now\n> wonder what should I do with the configuration's pixelformat fields though)\n> \n> >> \t}\n> >>\n> >> \t/*\n> >> @@ -281,26 +366,62 @@ 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> >> +\tfor (auto const &streamConfig : config) {\n> >> +\t\tStream *stream = streamConfig.first;\n> >> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> >>\n> >> -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\t\tif (isOutput(data, stream)) {\n> >> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >>\n> >> -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\n> >> +\t\t\tif (isViewfinderActive(data))\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\t/*\n> >> +\t\t\t * Configure viewfinder using the output stream\n> >> +\t\t\t * configuration if it is not part of the active\n> >> +\t\t\t * streams list.\n> >> +\t\t\t */\n> >> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\t\t} else if (isViewfinder(data, stream)) {\n> >> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\t\t\tif (isOutputActive(data))\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\t/*\n> >> +\t\t\t * Configure output using the viewfinder stream\n> >> +\t\t\t * configuration if it is not part of the active\n> >> +\t\t\t * streams list.\n> >> +\t\t\t */\n> >> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\t\t}\n> >> +\t}\n> >\n> > This looks quite complicated. I think you can configure the streams\n> > included in the configuration in the loop without caring about their\n> > type, and then configure the streams that are not included outside of\n> > the loop. The code would look simpler.\n> \n> Niklas had a similar idea, I'll try to see how it looks like.\n> \n> >> \treturn 0;\n> >> }\n> >> @@ -404,7 +525,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> >> +\tStream *stream = &data->outStream_;\n> >>\n> >> \t/* Queue a buffer to the ImgU output for capture. */\n> >> \tBuffer *buffer = request->findBuffer(stream);\n> >> @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> >> \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> >> \t\tstd::unique_ptr data =\n> >> \t\t\tutils::make_unique(this);\n> >> -\t\tstd::set streams{ &data->stream_ };\n> >> +\t\tstd::set 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);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 36E0560B2E\n\tfor ;\n\tMon, 8 Apr 2019 15:03:08 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-86-94-nat.elisa-mobile.fi\n\t[85.76.86.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2C500587;\n\tMon, 8 Apr 2019 15:03:07 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554728587;\n\tbh=nWwUlEgIZOnubgjwRb0Fxeac1fmwTmtmX/V/9QYqsuc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kBfmw1Q88JlFKhuvzme9BhxhTROOjG17O1S4zvR5awIpNPgkVUEpLMyoNulxv1s1D\n\tF6woDVIUs5os1QY/gRy8ffGjo5zTRSXk5fV94PiVSf2ki6gHniBx1UWewpgTpvftyK\n\t5s5sYMYpms8MYV5kX6diOgTPAIbp+j00ACyXNhq4=","Date":"Mon, 8 Apr 2019 16:02:56 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408130256.GC4888@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-jacopo@jmondi.org>\n\t<20190405154405.GA4636@pendragon.ideasonboard.com>\n\t<20190408075408.ibf4rqpkcudmo32z@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190408075408.ibf4rqpkcudmo32z@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 08 Apr 2019 13:03:08 -0000"}},{"id":1316,"web_url":"https://patchwork.libcamera.org/comment/1316/","msgid":"<20190408132330.cetj5jslf3luyjs4@uno.localdomain>","date":"2019-04-08T13:23:30","subject":"Re: [libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera\n\twith 2 streams","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Apr 08, 2019 at 04:02:56PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you.\n>\n\n[snip]\n\n> > >> +\tconfigs[&data->vfStream_] = config;\n> > >\n> > > Can both streams scale ?\n> >\n> > Can't they?\n>\n> I don't know, it was a true question :-)\n>\n> > Why is it relevant here?\n>\n> Because you're configuring both streams with the same output size,\n> different than the sensor size, so you need the scale on both streams.\n>\n\nOh, now I get what you mean here... Yes, both streams can scale\nindeed. According to upstream IPU3 developers viewfinder could\nadditionally perform composition, but the field of view selection\nprocedure is handled internally by the driver, as their API\nimplementation currently does not allow to apply a composition\nrectangle to the viewfinder's pad)\n\nThanks\n j\n\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 'viewfinder' 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> > >> \treturn configs;\n> > >> }\n> > >>\n> > >> int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > >> -\t\t\t\t\t std::map &config)\n> > >> +\t\t\t\t\t std::map > >> +\t\t\t\t\t\t StreamConfiguration> &config)\n> > >\n> > > That doesn't look nice, I think you could keep the longer line instead.\n> >\n> > I like 80-cols :(\n>\n> So do I, but something small exceptions are useful too :-)\n>\n> > >> {\n> > >> \tIPU3CameraData *data = cameraData(camera);\n> > >> -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> > >> +\tStreamConfiguration CIO2Config = {};\n> > >\n> > > Variables should start with lower case.\n> >\n> > Ack.. cio2Config then\n> >\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> > >> +\t/* Remove previously configured stream masks to store the new ones. */\n> > >> +\tdata->activeStreamsMask = 0;\n> > >> +\tfor (auto const &streamConfig : config) {\n> > >> +\t\tStream *stream = streamConfig.first;\n> > >> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\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 alignement\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> > >\n> > > While at it, s/consider/Consider/ and s/32\"/32\"./\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<< \"Requested image format \" << cfg.width << \"x\"\n> > >> +\t\t\t<< cfg.height << \"-0x\" << std::hex << std::setw(8)\n> > >> +\t\t\t<< cfg.pixelFormat << \" on camera'\"\n> > >> +\t\t\t<< camera->name() << \"'\";\n> > >\n> > > That will be a bit confusing if you don't print the stream name, again,\n> > > with a Stream subclass, you could easily get the name :-)\n> > >\n> > >> +\n> > >> +\t\t/*\n> > >> +\t\t * FIXME: As viewfinder should be operated even when\n> > >> +\t\t * applications do not intend to use it, we need to keep track\n> > >> +\t\t * of which streams have to be configured, to make meaningful\n> > >> +\t\t * decisions at configure and request queueing time.\n> > >> +\t\t *\n> > >> +\t\t * Walk here all the streams to configure and collect the\n> > >> +\t\t * active ones in a bitmaks.\n> > >> +\t\t */\n> > >> +\t\tif (isOutput(data, stream))\n> > >> +\t\t\tsetOutputActive(data);\n> > >> +\t\tif (isViewfinder(data, stream))\n> > >> +\t\t\tsetViewfinderActive(data);\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> > > Sounds good, but it's a bit strange to store that in a\n> > > StreamConfiguration, given that you only use it to configure the CIO2\n> > > and the ImgU input. How about replacing that by Size ?\n> >\n> > That's also a possibility, I only use the width and height (I now\n> > wonder what should I do with the configuration's pixelformat fields though)\n> >\n> > >> \t}\n> > >>\n> > >> \t/*\n> > >> @@ -281,26 +366,62 @@ 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> > >> +\tfor (auto const &streamConfig : config) {\n> > >> +\t\tStream *stream = streamConfig.first;\n> > >> +\t\tconst StreamConfiguration &cfg = streamConfig.second;\n> > >>\n> > >> -\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> +\t\tif (isOutput(data, stream)) {\n> > >> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >>\n> > >> -\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\n> > >> +\n> > >> +\t\t\tif (isViewfinderActive(data))\n> > >> +\t\t\t\tcontinue;\n> > >> +\n> > >> +\t\t\t/*\n> > >> +\t\t\t * Configure viewfinder using the output stream\n> > >> +\t\t\t * configuration if it is not part of the active\n> > >> +\t\t\t * streams list.\n> > >> +\t\t\t */\n> > >> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\t\t} else if (isViewfinder(data, stream)) {\n> > >> +\t\t\tret = imgu->configureOutput(&imgu->viewfinder_, cfg);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\n> > >> +\t\t\tif (isOutputActive(data))\n> > >> +\t\t\t\tcontinue;\n> > >> +\n> > >> +\t\t\t/*\n> > >> +\t\t\t * Configure output using the viewfinder stream\n> > >> +\t\t\t * configuration if it is not part of the active\n> > >> +\t\t\t * streams list.\n> > >> +\t\t\t */\n> > >> +\t\t\tret = imgu->configureOutput(&imgu->output_, cfg);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\n> > >> +\t\t\tret = imgu->configureOutput(&imgu->stat_, cfg);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\t\t}\n> > >> +\t}\n> > >\n> > > This looks quite complicated. I think you can configure the streams\n> > > included in the configuration in the loop without caring about their\n> > > type, and then configure the streams that are not included outside of\n> > > the loop. The code would look simpler.\n> >\n> > Niklas had a similar idea, I'll try to see how it looks like.\n> >\n> > >> \treturn 0;\n> > >> }\n> > >> @@ -404,7 +525,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> > >> +\tStream *stream = &data->outStream_;\n> > >>\n> > >> \t/* Queue a buffer to the ImgU output for capture. */\n> > >> \tBuffer *buffer = request->findBuffer(stream);\n> > >> @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()\n> > >> \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > >> \t\tstd::unique_ptr data =\n> > >> \t\t\tutils::make_unique(this);\n> > >> -\t\tstd::set streams{ &data->stream_ };\n> > >> +\t\tstd::set 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>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B15060004\n\tfor ;\n\tMon, 8 Apr 2019 15:22:42 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id A0B7EE0013;\n\tMon, 8 Apr 2019 13:22:41 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Apr 2019 15:23:30 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408132330.cetj5jslf3luyjs4@uno.localdomain>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-3-jacopo@jmondi.org>\n\t<20190405154405.GA4636@pendragon.ideasonboard.com>\n\t<20190408075408.ibf4rqpkcudmo32z@uno.localdomain>\n\t<20190408130256.GC4888@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"674co2sgbmp5mq4l\"","Content-Disposition":"inline","In-Reply-To":"<20190408130256.GC4888@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 08 Apr 2019 13:22:42 -0000"}}]