[libcamera-devel] [PATCH v2 03/16] libcamera: pipeline_handler: Simplify request completion

[Jacopo Mondi]

HI Laurent,

On Sat, Jul 13, 2019 at 08:23:38PM +0300, Laurent Pinchart wrote:
> libcamera guarantees that requests complete in sequence. This
> requirement is currently pushed down to pipeline handlers. Three out of
> four of our pipeline handlers implement that requirement based on the
> sole assumption that buffers will always complete in sequeuence, while
> the IPU3 pipeline handler implements a more complex logic.
>
> It turns out that the logic can be moved to the base PipelineHandler
> class with support from the Request class. Do so to simplify the
> pipeline handlers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 10 ++-------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  4 +---
>  src/libcamera/pipeline/uvcvideo.cpp      |  2 +-
>  src/libcamera/pipeline/vimc.cpp          |  2 +-
>  src/libcamera/pipeline_handler.cpp       | 26 ++++++++++++++++--------
>  5 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e4efb9722f76..1abd20e5fee5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -927,14 +927,8 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
>  		/* Request not completed yet, return here. */
>  		return;
>
> -	/* Complete the pending requests in queueing order. */
> -	while (1) {
> -		request = queuedRequests_.front();
> -		if (request->hasPendingBuffers())
> -			break;
> -
> -		pipe_->completeRequest(camera_, request);
> -	}
> +	/* Mark the request as complete. */
> +	pipe_->completeRequest(camera_, request);
>  }
>
>  /**
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4a5898d25f91..383236435a7f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -491,9 +491,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
>  {
>  	ASSERT(activeCamera_);
> -
> -	RkISP1CameraData *data = cameraData(activeCamera_);
> -	Request *request = data->queuedRequests_.front();
> +	Request *request = buffer->request();
>
>  	completeBuffer(activeCamera_, request, buffer);
>  	completeRequest(activeCamera_, request);
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 72313cfd246f..387d71ef567c 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -382,7 +382,7 @@ int UVCCameraData::init(MediaEntity *entity)
>
>  void UVCCameraData::bufferReady(Buffer *buffer)
>  {
> -	Request *request = queuedRequests_.front();
> +	Request *request = buffer->request();
>
>  	pipe_->completeBuffer(camera_, request, buffer);
>  	pipe_->completeRequest(camera_, request);
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index f8a58be060bb..ff2529c974fd 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -376,7 +376,7 @@ int VimcCameraData::init(MediaDevice *media)
>
>  void VimcCameraData::bufferReady(Buffer *buffer)
>  {
> -	Request *request = queuedRequests_.front();
> +	Request *request = buffer->request();
>
>  	pipe_->completeBuffer(camera_, request, buffer);
>  	pipe_->completeRequest(camera_, request);
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 0283e4e5ad51..c609200252f1 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -391,8 +391,9 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)
>   * This method shall be called by pipeline handlers to signal completion of the
>   * \a buffer part of the \a request. It notifies applications of buffer
>   * completion and updates the request's internal buffer tracking. The request
> - * is not completed automatically when the last buffer completes, pipeline
> - * handlers shall complete requests explicitly with completeRequest().
> + * is not completed automatically when the last buffer completes to give
> + * pipeline handlers a chance to perform any operation that may still be
> + * needed. They shall complete requests explicitly with completeRequest().
>   *
>   * \return True if all buffers contained in the request have completed, false
>   * otherwise
> @@ -413,17 +414,24 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,
>   * request has completed. The request is deleted and shall not be accessed once
>   * this method returns.
>   *
> - * The pipeline handler shall ensure that requests complete in the same order
> - * they are submitted.
> + * This method ensures that requests will be returned to the application in
> + * submission order, the pipeline handler may call it on any complete request
> + * without any ordering constraint.
>   */
>  void PipelineHandler::completeRequest(Camera *camera, Request *request)
>  {
> -	CameraData *data = cameraData(camera);
> -	ASSERT(request == data->queuedRequests_.front());
> -	data->queuedRequests_.pop_front();
> -
>  	request->complete(Request::RequestComplete);
> -	camera->requestComplete(request);
> +

Nit: empty line

> +	CameraData *data = cameraData(camera);
> +
> +	while (!data->queuedRequests_.empty()) {
> +		request = data->queuedRequests_.front();
> +		if (request->hasPendingBuffers())
> +			break;
> +
> +		data->queuedRequests_.pop_front();
> +		camera->requestComplete(request);
> +	}
>  }
>
>  /**
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/20190714/88949b20/attachment.sig>