[{"id":1183,"web_url":"https://patchwork.libcamera.org/comment/1183/","msgid":"<20190402103600.GL4805@pendragon.ideasonboard.com>","date":"2019-04-02T10:36:00","subject":"Re: [libcamera-devel] [PATCH v5 14/19] libcamera: ipu3: Implement\n\tcamera start/stop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Mar 26, 2019 at 09:38:57AM +0100, Jacopo Mondi wrote:\n> Start and stop video devices in the pipeline.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++++++++++++++++++++--\n> 1 file changed, 87 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index d3519bb1d536..5b3c44174566 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -111,6 +111,9 @@ public:\n> \tint exportBuffers(enum OutputId id, unsigned int count);\n> \tvoid freeBuffers();\n> \n> +\tint start();\n> +\tvoid stop();\n> +\n> \tunsigned int index_;\n> \tstd::string name_;\n> \tMediaDevice *media_;\n> @@ -154,6 +157,9 @@ public:\n> \tBufferPool *exportBuffers();\n> \tvoid freeBuffers();\n> \n> +\tint start();\n> +\tvoid stop();\n> +\n> \tV4L2Device *output_;\n> \tV4L2Subdevice *csi2_;\n> \tV4L2Subdevice *sensor_;\n> @@ -371,12 +377,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n> int PipelineHandlerIPU3::start(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tV4L2Device *cio2 = data->cio2_.output_;\n> +\tCIO2Device *cio2 = &data->cio2_;\n> +\tImgUDevice *imgu = data->imgu_;\n> \tint ret;\n> \n> -\tret = cio2->streamOn();\n> +\t/*\n> +\t * Start the ImgU video devices, buffers will be queued to the\n> +\t * ImgU output and viewfinder when requests will be queued.\n> +\t */\n> +\tret = cio2->start();\n> +\tif (ret) {\n> +\t\tcio2->stop();\n\nIs this needed ?\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = imgu->start();\n> \tif (ret) {\n> -\t\tLOG(IPU3, Info) << \"Failed to start camera \" << camera->name();\n\nI think this message would be useful to keep. The start() functions log\nthe detailed error cause (either directly, or in the functions they\ncall), but an overall message stating that the camera failed to start is\nuseful. You can use a goto error to avoid duplicating the message in the\ncio2 and imgu start error paths.\n\n> +\t\timgu->stop();\n> +\t\tcio2->stop();\n> \t\treturn ret;\n> \t}\n> \n> @@ -386,10 +404,9 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> void PipelineHandlerIPU3::stop(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tV4L2Device *cio2 = data->cio2_.output_;\n> \n> -\tif (cio2->streamOff())\n> -\t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n> +\tdata->cio2_.stop();\n> +\tdata->imgu_->stop();\n\nSimilarly I think an error message would be useful here too (but without\nreturning early, we should stop the imgu even if the cio2 fails to\nstop). As we don't care about the error code the code below should o.\n\n\tret = data->cio2_.stop();\n\tret |= data->imgu_->stop();\n\tif (ret)\n\t\tLOG(IPU3, Warning) << \"Failed to stop camera \" << camera->name();\n\n> \n> \tPipelineHandler::stop(camera);\n> }\n> @@ -833,6 +850,46 @@ void ImgUDevice::freeBuffers()\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU input buffers\";\n> }\n> \n> +int ImgUDevice::start()\n> +{\n> +\tint ret;\n> +\n> +\t/* Start the ImgU video devices. */\n> +\tret = output_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU output\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = viewfinder_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU viewfinder\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = stat_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU stat\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = input_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU input\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void ImgUDevice::stop()\n> +{\n> +\toutput_->streamOff();\n> +\tviewfinder_->streamOff();\n> +\tstat_->streamOff();\n> +\tinput_->streamOff();\n> +}\n> +\n> /*------------------------------------------------------------------------------\n> * CIO2 Device\n> */\n> @@ -1026,6 +1083,30 @@ void CIO2Device::freeBuffers()\n> \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> }\n> \n> +int CIO2Device::start()\n> +{\n> +\tint ret;\n> +\n> +\tfor (Buffer &buffer : pool_.buffers()) {\n> +\t\tret = output_->queueBuffer(&buffer);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\tret = output_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start CIO2\";\n\nThe streamOn() function already logs a message, I think you can remove\nthis one.\n\nWith those small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CIO2Device::stop()\n> +{\n> +\toutput_->streamOff();\n> +}\n> +\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> \n> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78366600FB\n\tfor ;\n\tTue, 2 Apr 2019 12:36:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DF8982F9;\n\tTue, 2 Apr 2019 12:36:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554201371;\n\tbh=XzDH1juGghI+Wds7moZ17fKTUbeyzcZLuUriP5KEa1U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MfrZ6cPpPC72MjTHeoo142OQC5lUngsGbl3q6e2MxfzDDunEvNxMooI3PAFgihAlr\n\tfcpaFJM14f1hko3avRQ5V/PusajFcQK9B/HQIJjgbv+cMUfFxxERW/7ZjJaEKYWrzt\n\tDhivUABp9xkwDfAcw0eyK1qBaWdtEwt3whcWscN8=","Date":"Tue, 2 Apr 2019 13:36:00 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402103600.GL4805@pendragon.ideasonboard.com>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-15-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190326083902.26121-15-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 14/19] libcamera: ipu3: Implement\n\tcamera start/stop","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":"Tue, 02 Apr 2019 10:36:11 -0000"}},{"id":1195,"web_url":"https://patchwork.libcamera.org/comment/1195/","msgid":"<20190402113627.GO23466@bigcity.dyn.berto.se>","date":"2019-04-02T11:36:27","subject":"Re: [libcamera-devel] [PATCH v5 14/19] libcamera: ipu3: Implement\n\tcamera start/stop","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-03-26 09:38:57 +0100, Jacopo Mondi wrote:\n> Start and stop video devices in the pipeline.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++++++++++++++++++++--\n> 1 file changed, 87 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index d3519bb1d536..5b3c44174566 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -111,6 +111,9 @@ public:\n> \tint exportBuffers(enum OutputId id, unsigned int count);\n> \tvoid freeBuffers();\n> \n> +\tint start();\n> +\tvoid stop();\n> +\n> \tunsigned int index_;\n> \tstd::string name_;\n> \tMediaDevice *media_;\n> @@ -154,6 +157,9 @@ public:\n> \tBufferPool *exportBuffers();\n> \tvoid freeBuffers();\n> \n> +\tint start();\n> +\tvoid stop();\n> +\n> \tV4L2Device *output_;\n> \tV4L2Subdevice *csi2_;\n> \tV4L2Subdevice *sensor_;\n> @@ -371,12 +377,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n> int PipelineHandlerIPU3::start(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tV4L2Device *cio2 = data->cio2_.output_;\n> +\tCIO2Device *cio2 = &data->cio2_;\n> +\tImgUDevice *imgu = data->imgu_;\n> \tint ret;\n> \n> -\tret = cio2->streamOn();\n> +\t/*\n> +\t * Start the ImgU video devices, buffers will be queued to the\n> +\t * ImgU output and viewfinder when requests will be queued.\n> +\t */\n> +\tret = cio2->start();\n> +\tif (ret) {\n> +\t\tcio2->stop();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = imgu->start();\n> \tif (ret) {\n> -\t\tLOG(IPU3, Info) << \"Failed to start camera \" << camera->name();\n> +\t\timgu->stop();\n> +\t\tcio2->stop();\n> \t\treturn ret;\n> \t}\n> \n> @@ -386,10 +404,9 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> void PipelineHandlerIPU3::stop(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tV4L2Device *cio2 = data->cio2_.output_;\n> \n> -\tif (cio2->streamOff())\n> -\t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n> +\tdata->cio2_.stop();\n> +\tdata->imgu_->stop();\n\nNot complaining only pointing out it looks odd that one is a pointer and \nthe other one not :-)\n\nWith Laurents comments addressed feel free to add,\n\nReviewed-by: Niklas Söderlund \n\n> \n> \tPipelineHandler::stop(camera);\n> }\n> @@ -833,6 +850,46 @@ void ImgUDevice::freeBuffers()\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU input buffers\";\n> }\n> \n> +int ImgUDevice::start()\n> +{\n> +\tint ret;\n> +\n> +\t/* Start the ImgU video devices. */\n> +\tret = output_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU output\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = viewfinder_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU viewfinder\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = stat_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU stat\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = input_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU input\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void ImgUDevice::stop()\n> +{\n> +\toutput_->streamOff();\n> +\tviewfinder_->streamOff();\n> +\tstat_->streamOff();\n> +\tinput_->streamOff();\n> +}\n> +\n> /*------------------------------------------------------------------------------\n> * CIO2 Device\n> */\n> @@ -1026,6 +1083,30 @@ void CIO2Device::freeBuffers()\n> \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> }\n> \n> +int CIO2Device::start()\n> +{\n> +\tint ret;\n> +\n> +\tfor (Buffer &buffer : pool_.buffers()) {\n> +\t\tret = output_->queueBuffer(&buffer);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\tret = output_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start CIO2\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CIO2Device::stop()\n> +{\n> +\toutput_->streamOff();\n> +}\n> +\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> \n> } /* namespace libcamera */\n> -- \n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6215760DB3\n\tfor ;\n\tTue, 2 Apr 2019 13:36:29 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id u21so8740956lfu.2\n\tfor ;\n\tTue, 02 Apr 2019 04:36:29 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ti24sm2705220ljb.31.2019.04.02.04.36.27\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 02 Apr 2019 04:36:27 -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=56+EwTsFVMGTi6P0yZPOtFOiDOLCffvGl4Ss+wJvg0g=;\n\tb=IFrmXNlu/4FSwPNKPHGVVBwA8Hiz90e9KFe6Y2ts9YztrLTKR573X0hErjHMSKmMt5\n\tdvpaAbvHBYzwS6X0En+tAte2QHqW1A03XmT3hpOMSLSW0tNJaDE4KjqQFYwB1wJITiam\n\t0gboBolO+MNWTZ2cQ5eNP2Mgm10lQMgWZud2xuhTTiToGC4J7SwWp8fLAreurAzlI05h\n\tD2gqukgkf8bjdPuE5jsAsWHl5zSbDq2mLE73BFqRHU9igqW2u7Ed/2tilPGEXMrZgz2V\n\tL7S/sXIrm1m/d7Rai8KzP6Pao5NRiiT6RveD6tAJiZNXuIXg0RPHW0jiI8BLM8MiAoQl\n\tJzjw==","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=56+EwTsFVMGTi6P0yZPOtFOiDOLCffvGl4Ss+wJvg0g=;\n\tb=L/M5ecf+vWuOLwXNtfE6gsQ2/AXbFSPL1ZQNONFEK/Wq+GrRNCyCjkcPKGRo2LDL5C\n\tFt37S8tKyG2dtdL7pdq3hMRvGS4QanlFfggC9obg/00FqGof8nmUGcU7bQwvPyzgMTkg\n\txCgbSqMBh7iP+a5XFs4zL3TZ5CBWtr6XqFmHA9MdF6Nrb5NpQ3i0fIio0i0Y0LF992qw\n\tBWIGK1tI66AfiMvg6dS6Y7wBpJZNHKTodp1GIni/+zSsccef8anUVhs7ScDyP4Vz76P2\n\tvUJjahYQYBIdqZNekiC15ir42ukDF0tYIZJVLsrxbEVMtJDCkwM8FqvlecYL+OF5VKkP\n\tohqA==","X-Gm-Message-State":"APjAAAUwvzN11HFRb9h86maVvdv8FfwbvoZIveSnwxUdQMb3nwViWt2O\n\tYqK/5c3vO1jgx8ZLoe7XMxoYXNAGEVY=","X-Google-Smtp-Source":"APXvYqwPr8FrC2WYmeaLDVAKSmAm2ASYVZ3mhs86jhrvzrz8/hzcmiYPs4KGHwy80jez2iXU4o0MMg==","X-Received":"by 2002:a19:f203:: with SMTP id q3mr5340374lfh.78.1554204988832; \n\tTue, 02 Apr 2019 04:36:28 -0700 (PDT)","Date":"Tue, 2 Apr 2019 13:36:27 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402113627.GO23466@bigcity.dyn.berto.se>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-15-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":"<20190326083902.26121-15-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v5 14/19] libcamera: ipu3: Implement\n\tcamera start/stop","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":"Tue, 02 Apr 2019 11:36:29 -0000"}}]