[libcamera-devel] [PATCH 04/13] libcamera: pipeline: rkisp1: Prepare buffer ready handlers for multiple streams

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Aug 13, 2020 at 02:52:37AM +0200, Niklas Söderlund wrote:
> The buffer ready handlers are designed for a single application facing
> stream from the main path. To prepare for multiple application facing
> streams from main and/or self path the handlers need to be prepared.
> 
> The data keeping tasks of frame number and advancing the timeline can be
> moved from the application facing buffer ready handler to the statistics
> handler. For each request processed there will always be a statistic
> buffer and as the ISP is inline and is the source of both main, self and
> statistic paths there is no lose in precision from this.
> 
> The application facing handler no longer needs a special case for
> cancelled frames and can be made simpler. With this change the handlers
> are ready to deal with any combinations of application facing streams.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 38382a6894dac22a..a1cda12d244f2d97 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1074,20 +1074,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
> -	RkISP1CameraData *data = cameraData(activeCamera_);
>  	Request *request = buffer->request();
>  
> -	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> -		completeBuffer(activeCamera_, request, buffer);
> -		completeRequest(activeCamera_, request);
> -		return;
> -	}
> -
> -	data->timeline_.bufferReady(buffer);
> -
> -	if (data->frame_ <= buffer->metadata().sequence)
> -		data->frame_ = buffer->metadata().sequence + 1;
> -
>  	completeBuffer(activeCamera_, request, buffer);
>  	tryCompleteRequest(request);
>  }
> @@ -1118,6 +1106,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  	if (!info)
>  		return;
>  
> +	data->timeline_.bufferReady(buffer);
> +
> +	if (data->frame_ <= buffer->metadata().sequence)
> +		data->frame_ = buffer->metadata().sequence + 1;
> +

This should work for now, but may cause issues later, as we should fill
the request with metadata before completing it. That will however
require more rework, so, for now,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	IPAOperationData op;
>  	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
>  	op.data = { info->frame, info->statBuffer->cookie() };

-- 
Regards,

Laurent Pinchart