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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Apr 18, 2019 at 12:47:13PM +0200, Jacopo Mondi wrote:
> 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.

You don't rename the method anymore in v5, have you forgotten to update
the commit message ?

> 
> 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 | 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 ee490a488cf7..71b36ecf6cd9 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -705,6 +705,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);
>  
>  		/* Initialize and register the Camera and its streams. */
>  		std::string cameraName = cio2->sensor_->entityName() + " "
> @@ -750,10 +752,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;

Unless I'm mistaken you were able to get the main output working without
needing to queue dummy buffers, is this check still needed ?

> +
> +	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.

s/post-pone/postpone/

> +	 */
> +	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) {
> +		if (front->hasPendingBuffers())
> +			break;
> +
> +		pipe_->completeRequest(camera_, front);
> +		front = queuedRequests_.front();
> +	}

Would the following be simpler ?

	/* Complete the pending requests in queuing order. */
	while (1) {
		request = queuedRequests_.front();
		if (request->hasPendingBuffers())
			break;

		pipe_->completeRequest(camera_, request);
	}

>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart