[{"id":1467,"web_url":"https://patchwork.libcamera.org/comment/1467/","msgid":"<20190418204335.GD4794@pendragon.ideasonboard.com>","date":"2019-04-18T20:43:35","subject":"Re: [libcamera-devel] [PATCH v6 5/6] libcamera: ipu3: Connect\n\tviewfinder's BufferReady signal","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 Thu, Apr 18, 2019 at 06:46:37PM +0200, Jacopo Mondi wrote:\n> The viewfinder and main output require identical logic for buffer and\n> request completion. Connect the viewfinder bufferReady signal to the slot\n> and handle requests for both main output and viewfinder there.\n> \n> Update the slot logic to ignore internal buffers that are not part of\n> the request,\n\nIs this still needed ?\n\n> and to complete the request only when the last buffer completes.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++---\n> 1 file changed, 29 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8353272642bd..647b4f4f365f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t\t\t\t&IPU3CameraData::imguInputBufferReady);\n> \t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> +\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> +\t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> \n> \t\t/* Create and register the Camera instance. */\n> \t\tstd::string cameraName = cio2->sensor_->entity()->name() + \" \"\n> @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> */\n> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n> +\tif (!request)\n> +\t\t/* Completed buffers not part of a request are ignored. */\n> +\t\treturn;\n\nNow that you don't queue dummy buffers anymore, can this still happen ?\n\n> +\n> +\tif (!pipe_->completeBuffer(camera_, request, buffer))\n> +\t\t/* Request not completed yet, return here. */\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Complete requests in queuing order: if some other request is\n> +\t * pending, post-pone completion.\n> +\t */\n> +\tRequest *front = queuedRequests_.front();\n> +\tif (front != request)\n> +\t\treturn;\n> \n> -\tpipe_->completeBuffer(camera_, request, buffer);\n> -\tpipe_->completeRequest(camera_, request);\n> +\t/*\n> +\t * Complete the current request, and all the other pending ones,\n> +\t * in queuing order.\n> +\t */\n> +\twhile (1) {\n\nI think you've missed my review of v5.\n\n> +\t\tif (front->hasPendingBuffers())\n> +\t\t\tbreak;\n> +\n> +\t\tpipe_->completeRequest(camera_, front);\n> +\t\tfront = queuedRequests_.front();\n> +\t}\n> }\n> \n> /**","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 20BA5600F9\n\tfor ;\n\tThu, 18 Apr 2019 22:43:45 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 74564333;\n\tThu, 18 Apr 2019 22:43:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555620224;\n\tbh=UVDmfUr8YIxsCRJDLCa0N+T79h/r7tmKH6d2cdtWCzs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DW3BQC845pnTd3A/EZ5LwVs/ZlNDSXzhyigeyx/cExbrik4Fj9tcBGqILT5KJ0onb\n\tr1bt16oiiXwfaINdQR6cun5n1xZFx/EkPYb4ILp23GAzEs+DfUF93CHkgKIDbToMSx\n\tDQvXQe89IsWnvE+lCIMOzHdyynjeasqN3n3q2/7g=","Date":"Thu, 18 Apr 2019 23:43:35 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190418204335.GD4794@pendragon.ideasonboard.com>","References":"<20190418164638.400-1-jacopo@jmondi.org>\n\t<20190418164638.400-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190418164638.400-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 5/6] libcamera: ipu3: Connect\n\tviewfinder's BufferReady signal","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":"Thu, 18 Apr 2019 20:43:45 -0000"}},{"id":1471,"web_url":"https://patchwork.libcamera.org/comment/1471/","msgid":"<20190419065115.jnked2ihvnsg2vax@uno.localdomain>","date":"2019-04-19T06:51:15","subject":"Re: [libcamera-devel] [PATCH v6 5/6] libcamera: ipu3: Connect\n\tviewfinder's BufferReady signal","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Apr 18, 2019 at 11:43:35PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 18, 2019 at 06:46:37PM +0200, Jacopo Mondi wrote:\n> > The viewfinder and main output require identical logic for buffer and\n> > request completion. Connect the viewfinder bufferReady signal to the slot\n> > and handle requests for both main output and viewfinder there.\n> >\n> > Update the slot logic to ignore internal buffers that are not part of\n> > the request,\n>\n> Is this still needed ?\n>\n\nAgain no, as the below code handling this case.\n\n> > and to complete the request only when the last buffer completes.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++---\n> > 1 file changed, 29 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 8353272642bd..647b4f4f365f 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t\t\t\t&IPU3CameraData::imguInputBufferReady);\n> > \t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > +\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> > +\t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> >\n> > \t\t/* Create and register the Camera instance. */\n> > \t\tstd::string cameraName = cio2->sensor_->entity()->name() + \" \"\n> > @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> > */\n> > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> > {\n> > -\tRequest *request = queuedRequests_.front();\n> > +\tRequest *request = buffer->request();\n> > +\tif (!request)\n> > +\t\t/* Completed buffers not part of a request are ignored. */\n> > +\t\treturn;\n>\n> Now that you don't queue dummy buffers anymore, can this still happen ?\n>\n\nIt should not.\n\n> > +\n> > +\tif (!pipe_->completeBuffer(camera_, request, buffer))\n> > +\t\t/* Request not completed yet, return here. */\n> > +\t\treturn;\n> > +\n> > +\t/*\n> > +\t * Complete requests in queuing order: if some other request is\n> > +\t * pending, post-pone completion.\n> > +\t */\n> > +\tRequest *front = queuedRequests_.front();\n> > +\tif (front != request)\n> > +\t\treturn;\n> >\n> > -\tpipe_->completeBuffer(camera_, request, buffer);\n> > -\tpipe_->completeRequest(camera_, request);\n> > +\t/*\n> > +\t * Complete the current request, and all the other pending ones,\n> > +\t * in queuing order.\n> > +\t */\n> > +\twhile (1) {\n>\n> I think you've missed my review of v5.\n>\n\nThis one on v4 you mean?\n\n-------------------------------------------------------------------------\n\nI think you can simplify all this with\n\n /* Complete requests in queuing order. */\n while (1) {\n request = queuedRequest_.front();\n if (!request->empty())\n break;\n\n pipe_->completeRequest(camera_, request);\n------------------------------------------------------------------------\n\nI might have missed why it is better, considering here above I have to\ncheck (front == request) to proceed to complete it.\n\nThanks\n j\n\n> > +\t\tif (front->hasPendingBuffers())\n> > +\t\t\tbreak;\n> > +\n> > +\t\tpipe_->completeRequest(camera_, front);\n> > +\t\tfront = queuedRequests_.front();\n> > +\t}\n> > }\n> >\n> > /**\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA94A60B1B\n\tfor ;\n\tFri, 19 Apr 2019 08:50:22 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 0A15824000A;\n\tFri, 19 Apr 2019 06:50:21 +0000 (UTC)"],"Date":"Fri, 19 Apr 2019 08:51:15 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190419065115.jnked2ihvnsg2vax@uno.localdomain>","References":"<20190418164638.400-1-jacopo@jmondi.org>\n\t<20190418164638.400-6-jacopo@jmondi.org>\n\t<20190418204335.GD4794@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"2kaf3mq6faoshsxm\"","Content-Disposition":"inline","In-Reply-To":"<20190418204335.GD4794@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v6 5/6] libcamera: ipu3: Connect\n\tviewfinder's BufferReady signal","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, 19 Apr 2019 06:50:22 -0000"}},{"id":1483,"web_url":"https://patchwork.libcamera.org/comment/1483/","msgid":"<20190419104206.GB7006@pendragon.ideasonboard.com>","date":"2019-04-19T10:42:06","subject":"Re: [libcamera-devel] [PATCH v6 5/6] libcamera: ipu3: Connect\n\tviewfinder's BufferReady signal","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Apr 19, 2019 at 08:51:15AM +0200, Jacopo Mondi wrote:\n> On Thu, Apr 18, 2019 at 11:43:35PM +0300, Laurent Pinchart wrote:\n> > On Thu, Apr 18, 2019 at 06:46:37PM +0200, Jacopo Mondi wrote:\n> >> The viewfinder and main output require identical logic for buffer and\n> >> request completion. Connect the viewfinder bufferReady signal to the slot\n> >> and handle requests for both main output and viewfinder there.\n> >>\n> >> Update the slot logic to ignore internal buffers that are not part of\n> >> the request,\n> >\n> > Is this still needed ?\n> \n> Again no, as the below code handling this case.\n> \n> >> and to complete the request only when the last buffer completes.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++---\n> >> 1 file changed, 29 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 8353272642bd..647b4f4f365f 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras()\n> >> \t\t\t\t\t&IPU3CameraData::imguInputBufferReady);\n> >> \t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> >> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> >> +\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> >> +\t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> >>\n> >> \t\t/* Create and register the Camera instance. */\n> >> \t\tstd::string cameraName = cio2->sensor_->entity()->name() + \" \"\n> >> @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> >> */\n> >> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> >> {\n> >> -\tRequest *request = queuedRequests_.front();\n> >> +\tRequest *request = buffer->request();\n> >> +\tif (!request)\n> >> +\t\t/* Completed buffers not part of a request are ignored. */\n> >> +\t\treturn;\n> >\n> > Now that you don't queue dummy buffers anymore, can this still happen ?\n> \n> It should not.\n> \n> >> +\n> >> +\tif (!pipe_->completeBuffer(camera_, request, buffer))\n> >> +\t\t/* Request not completed yet, return here. */\n> >> +\t\treturn;\n> >> +\n> >> +\t/*\n> >> +\t * Complete requests in queuing order: if some other request is\n> >> +\t * pending, post-pone completion.\n> >> +\t */\n> >> +\tRequest *front = queuedRequests_.front();\n> >> +\tif (front != request)\n> >> +\t\treturn;\n> >>\n> >> -\tpipe_->completeBuffer(camera_, request, buffer);\n> >> -\tpipe_->completeRequest(camera_, request);\n> >> +\t/*\n> >> +\t * Complete the current request, and all the other pending ones,\n> >> +\t * in queuing order.\n> >> +\t */\n> >> +\twhile (1) {\n> >\n> > I think you've missed my review of v5.\n> \n> This one on v4 you mean?\n> \n> -------------------------------------------------------------------------\n> \n> I think you can simplify all this with\n> \n> /* Complete requests in queuing order. */\n> while (1) {\n> request = queuedRequest_.front();\n> if (!request->empty())\n> break;\n> \n> pipe_->completeRequest(camera_, request);\n> ------------------------------------------------------------------------\n> \n> I might have missed why it is better, considering here above I have to\n> check (front == request) to proceed to complete it.\n\nThat's my point, I don't think you need that check, you can complete all\nrequests that have no pending buffer in order until you reach a request\nthat still has pending buffers.\n\n> >> +\t\tif (front->hasPendingBuffers())\n> >> +\t\t\tbreak;\n> >> +\n> >> +\t\tpipe_->completeRequest(camera_, front);\n> >> +\t\tfront = queuedRequests_.front();\n> >> +\t}\n> >> }\n> >>\n> >> /**","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 88A7860B2E\n\tfor ;\n\tFri, 19 Apr 2019 12:42:24 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E593A31A;\n\tFri, 19 Apr 2019 12:42:23 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555670544;\n\tbh=ChJYk3eZQPTOdvK1w+mhA2ZbQ11ZmWtOVvLxaxr+DHY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aOn2OdT/2Cg9d3MRTLC75OtJFIq81U3dHyPUPogxhiTPSVJlGPBRqUCkNT73S3J32\n\tfG7ViiQQUy/Hu6Jasry1k+Klu/MRQ5vXeI9GKRS3yHDACoNmDHkT6c/aLdbTda1JAq\n\tozTjO0eG1kvNz6M6WqlUbKq36ZuRM5zNSyZYHCGg=","Date":"Fri, 19 Apr 2019 13:42:06 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190419104206.GB7006@pendragon.ideasonboard.com>","References":"<20190418164638.400-1-jacopo@jmondi.org>\n\t<20190418164638.400-6-jacopo@jmondi.org>\n\t<20190418204335.GD4794@pendragon.ideasonboard.com>\n\t<20190419065115.jnked2ihvnsg2vax@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190419065115.jnked2ihvnsg2vax@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 5/6] libcamera: ipu3: Connect\n\tviewfinder's BufferReady signal","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, 19 Apr 2019 10:42:24 -0000"}}]