[{"id":1115,"web_url":"https://patchwork.libcamera.org/comment/1115/","msgid":"<20190323141355.GJ4587@pendragon.ideasonboard.com>","date":"2019-03-23T14:13:55","subject":"Re: [libcamera-devel] [PATCH v4 24/31] libcamera: ipu3: Queue\n\trequest for multiple 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, Mar 20, 2019 at 05:30:48PM +0100, Jacopo Mondi wrote:\n> Add support for queue request on the main and secondary outputs of the\n> ImgU.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 46 ++++++++++++++++------------\n> 1 file changed, 27 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index ff1e5329c83d..b2df9a4ac922 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -689,38 +689,46 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \n> int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> {\n> +\tstd::set streams = request->streams();\n\nMaybe\n\n\tconst std::set &streams = request->streams();\n\n?\n\n> \tIPU3CameraData *data = cameraData(camera);\n> \tV4L2Device *viewfinder = data->imgu->viewfinder;\n> \tV4L2Device *output = data->imgu->output;\n> \tV4L2Device *stat = data->imgu->stat;\n> -\tStream *stream = &data->streams_[0];\n> +\tImgUDevice *imgu = data->imgu;\n> \tBuffer *tmpBuffer;\n> \n> -\t/*\n> -\t * Queue buffer on VF and stat.\n> -\t * FIXME: this is an hack!\n> -\t */\n> -\ttmpBuffer = &data->imgu->vfPool.buffers()[tmpBufferCount];\n> -\tviewfinder->queueBuffer(tmpBuffer);\n> +\tfor (Stream *stream : streams) {\n> +\t\tBuffer *buffer = request->findBuffer(stream);\n\nThis clearly calls for exposing the stream to buffers map instead of\nimplementing a streams() method :-)\n\n> +\t\tif (!buffer) {\n> +\t\t\tLOG(IPU3, Error) << \"Attempt to queue invalid request\";\n> +\t\t\treturn -ENOENT;\n> +\t\t}\n> +\n> +\t\tV4L2Device *videoDevice = isOutput(data, stream)\n> +\t\t\t\t\t? output : viewfinder;\n> +\n> +\t\tint ret = videoDevice->queueBuffer(buffer);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> +\t\tif (isOutput(data, stream) && !isViewfinderActive(data)) {\n> +\t\t\ttmpBuffer = &imgu->vfPool.buffers()[tmpBufferCount];\n> +\t\t\tviewfinder->queueBuffer(tmpBuffer);\n> +\t\t}\n> \n> +\t\tif (isViewfinder(data, stream) && !isOutputActive(data)) {\n> +\t\t\ttmpBuffer = &imgu->outputPool.buffers()[tmpBufferCount];\n> +\t\t\toutput->queueBuffer(tmpBuffer);\n> +\t\t}\n\nI think the code would be clearer if you moved it out of the loop. How\nabout making a copy of the set of active streams, removing streams\npresent in the request from the copy, and then loop over the remaining\nstreams in the copy to queue the dummy buffers ?\n\n> +\t}\n> +\n> +\t/* Queue buffer on stat unconditionally. */\n> \ttmpBuffer = &data->imgu->statPool.buffers()[tmpBufferCount];\n> \tstat->queueBuffer(tmpBuffer);\n> \n> \ttmpBufferCount++;\n> \ttmpBufferCount %= IPU3_IMGU_BUFFER_COUNT;\n\nThis won't work as expected. If you have, let's say, 4 buffers for each\nstream, an application could queue 8 requests consecutively (4 requests\nwith the output stream only and 4 requests with the viewfinder stream\nonly). With 4 stats buffers, you'll have a problem.\n\n> -\t/* Queue a buffer to the ImgU output for capture. */\n> -\tBuffer *buffer = request->findBuffer(stream);\n> -\tif (!buffer) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Attempt to queue request with invalid stream\";\n> -\t\treturn -ENOENT;\n> -\t}\n> -\n> -\tint ret = output->queueBuffer(buffer);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> \tPipelineHandler::queueRequest(camera, request);\n> \n> \treturn 0;","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 A85F2610B6\n\tfor ;\n\tSat, 23 Mar 2019 15:14:08 +0100 (CET)","from pendragon.ideasonboard.com\n\t(p5269001-ipngn11702marunouchi.tokyo.ocn.ne.jp [114.158.195.1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 04C1449;\n\tSat, 23 Mar 2019 15:14:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1553350448;\n\tbh=gRpQ8npQ0Dq+nhJ4StCmVo7Tbq0JhXTvFq7cehLy3DA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fYYxUD/Z5C6plja1dUo9/iFkOYoIUhwEXqncpdzIijg5+Z65cYhvTdxPW03GCze8a\n\tTHsH/IoP7cCLHYjJB6MeDDGn7WPcqM3DdE/+H89R6EiMZu9JeaEw0SnROgicLotQZV\n\tDJbls3Sg8Qn70DyL9bGUg1I4Kk/S2mCf1u1oF9ww=","Date":"Sat, 23 Mar 2019 16:13:55 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190323141355.GJ4587@pendragon.ideasonboard.com>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-25-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190320163055.22056-25-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 24/31] libcamera: ipu3: Queue\n\trequest for multiple 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, 23 Mar 2019 14:14:08 -0000"}}]