[libcamera-devel] [PATCH v4 10/12] libcamera: ipu3: Connect viewfinder's BufferReady signal

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Apr 15, 2019 at 01:39:30PM +0200, Jacopo Mondi wrote:
> On Sun, Apr 14, 2019 at 10:32:29PM +0200, Niklas Söderlund wrote:
> > On 2019-04-09 21:25:46 +0200, Jacopo Mondi wrote:
> >> Connect the viewfinder buffer ready signal to the IPU3CameraData slot
> >> that complets the buffer first, and if not waiting for other buffers
> >> completes the request as well.

The commit message is a bit confusing. How about

The viewfinder and main output require identical logic for buffer and
request completion. Rename the IPU3CameraData::imguOutputBufferReady()
slot to IPU3CameraData::imguCaptureBufferReady() to reflect this, and
connect the viewfinder bufferReady signal to the slot.

Update the slot logic to ignore internal buffers that are not part of
the request, 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 | 43 ++++++++++++++++++++++++----
> >>  1 file changed, 37 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index bb8d4ce644ca..75ffdc56d157 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -182,7 +182,7 @@ private:
> >>  		{
> >>  		}
> >>
> >> -		void imguOutputBufferReady(Buffer *buffer);
> >> +		void imguCaptureBufferReady(Buffer *buffer);

By the way, I think it's fine if you keep the existing name, as it
matches imguInputBufferReady(). Up to you, I don't mind one way or the
other.

> >>  		void imguInputBufferReady(Buffer *buffer);
> >>  		void cio2BufferReady(Buffer *buffer);
> >>
> >> @@ -722,7 +722,9 @@ int PipelineHandlerIPU3::registerCameras()
> >>  		data->imgu_->input_->bufferReady.connect(data.get(),
> >>  					&IPU3CameraData::imguInputBufferReady);
> >>  		data->imgu_->output_.dev->bufferReady.connect(data.get(),
> >> -					&IPU3CameraData::imguOutputBufferReady);
> >> +					&IPU3CameraData::imguCaptureBufferReady);
> >> +		data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),
> >> +					&IPU3CameraData::imguCaptureBufferReady);
> >>
> >>
> >>  		/* Initialize and register the Camera and its streams. */
> >> @@ -769,12 +771,41 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> >>   *
> >>   * Buffers completed from the ImgU output are directed to the application.
> >>   */
> >> -void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> >> +void PipelineHandlerIPU3::IPU3CameraData::imguCaptureBufferReady(Buffer *buffer)
> >>  {
> >> -	Request *request = queuedRequests_.front();
> >> +	Request *request = buffer->request();
> 
> When commenting patch #9 you said you don't like this.
> I agree on the part that makes the 'request' and 'setRequest' method
> public and we might need to play with 'friend' to limit the
> accessibility of those method, but you also said you don't like how it
> is used. How would you like to see this being used ?
> 
> >> +	if (!request)
> >> +		/*
> >> +		 * Completed buffers not part of a request are ignored
> >> +		 * (they most probably come from the output stream
> >> +		 * internal pool)

Most probably ? Is there any other option ?

> >> +		 */
> >> +		return;
> >> +
> >> +	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;
> >
> > I'm not sure, but i think this needs to be moved to the framework
> > somehow. It will be slight variations of how this will be implemented in
> > different pipeline handlers if we leave it to each pipeline handler to
> > implement ordered completion of requests.
> 
> I agree, but not right now. We agreed when discussing v3 to do this
> for IPU3 and see how it looks like. The second 'real' pipeline handler
> we'll have we'll try to move this one layer up.

I guess I'll have to handle this then :-)

> > Best case it should happen transparent to the pipeline handlers, but
> > that might be a tall order. How about implementing a
> > PipelineHandler::completeReady() helper which implementations can call
> > which would take care of dequeuing requests in a ordered fashion? Or if
> > you have a nice idea for how to skip the helper and move to something
> > transparent that would be even better.
> >
> >> -	pipe_->completeBuffer(camera_, request, buffer);
> >> -	pipe_->completeRequest(camera_, request);
> >> +	/*
> >> +	 * Complete the current request, and all the other pending ones,
> >> +	 * in queuing order.
> >> +	 */
> >> +	while (1) {
> >> +		if (front->empty())
> >> +			pipe_->completeRequest(camera_, front);
> >> +		else
> >> +			break;
> >> +
> >> +		front = queuedRequests_.front();
> >> +	}

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);
	}

> >>  }
> >>
> >>  /**

-- 
Regards,

Laurent Pinchart