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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Apr 19, 2019 at 08:51:15AM +0200, Jacopo Mondi wrote:
> On Thu, Apr 18, 2019 at 11:43:35PM +0300, Laurent Pinchart wrote:
> > 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.

That's my point, I don't think you need that check, you can complete all
requests that have no pending buffer in order until you reach a request
that still has pending buffers.

> >> +		if (front->hasPendingBuffers())
> >> +			break;
> >> +
> >> +		pipe_->completeRequest(camera_, front);
> >> +		front = queuedRequests_.front();
> >> +	}
> >>  }
> >>
> >>  /**

-- 
Regards,

Laurent Pinchart