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

[Laurent Pinchart]

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 ?

> 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 ?

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

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

-- 
Regards,

Laurent Pinchart