[libcamera-devel] [PATCH v6 5/6] libcamera: ipu3: Connect viewfinder's BufferReady signal

[Jacopo Mondi]

Hi Laurent,

On Thu, Apr 18, 2019 at 11:43:35PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Apr 18, 2019 at 06:46:37PM +0200, Jacopo Mondi wrote:
> > The viewfinder and main output require identical logic for buffer and
> > request completion. Connect the viewfinder bufferReady signal to the slot
> > and handle requests for both main output and viewfinder there.
> >
> > Update the slot logic to ignore internal buffers that are not part of
> > the request,
>
> Is this still needed ?
>

Again no, as the below code handling this case.

> > and to complete the request only when the last buffer completes.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +++++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 8353272642bd..647b4f4f365f 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -729,6 +729,8 @@ int PipelineHandlerIPU3::registerCameras()
> >  					&IPU3CameraData::imguInputBufferReady);
> >  		data->imgu_->output_.dev->bufferReady.connect(data.get(),
> >  					&IPU3CameraData::imguOutputBufferReady);
> > +		data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),
> > +					&IPU3CameraData::imguOutputBufferReady);
> >
> >  		/* Create and register the Camera instance. */
> >  		std::string cameraName = cio2->sensor_->entity()->name() + " "
> > @@ -774,10 +776,34 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> >   */
> >  void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> >  {
> > -	Request *request = queuedRequests_.front();
> > +	Request *request = buffer->request();
> > +	if (!request)
> > +		/* Completed buffers not part of a request are ignored. */
> > +		return;
>
> Now that you don't queue dummy buffers anymore, can this still happen ?
>

It should not.

> > +
> > +	if (!pipe_->completeBuffer(camera_, request, buffer))
> > +		/* Request not completed yet, return here. */
> > +		return;
> > +
> > +	/*
> > +	 * Complete requests in queuing order: if some other request is
> > +	 * pending, post-pone completion.
> > +	 */
> > +	Request *front = queuedRequests_.front();
> > +	if (front != request)
> > +		return;
> >
> > -	pipe_->completeBuffer(camera_, request, buffer);
> > -	pipe_->completeRequest(camera_, request);
> > +	/*
> > +	 * Complete the current request, and all the other pending ones,
> > +	 * in queuing order.
> > +	 */
> > +	while (1) {
>
> I think you've missed my review of v5.
>

This one on v4 you mean?

-------------------------------------------------------------------------

I think you can simplify all this with

       /* Complete requests in queuing order. */
       while (1) {
               request = queuedRequest_.front();
               if (!request->empty())
                       break;

               pipe_->completeRequest(camera_, request);
------------------------------------------------------------------------

I might have missed why it is better, considering here above I have to
check (front == request) to proceed to complete it.

Thanks
   j

> > +		if (front->hasPendingBuffers())
> > +			break;
> > +
> > +		pipe_->completeRequest(camera_, front);
> > +		front = queuedRequests_.front();
> > +	}
> >  }
> >
> >  /**
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190419/c694673c/attachment.sig>