[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Use maxBuffers instead of count when importing buffers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Mar 20, 2020 at 12:44:01AM +0100, Niklas Söderlund wrote:
> When folding buffer management with start/stop the wrong variable was
> passed to importBuffers() resulting in only one buffer being imported
> for the video node making capture impossible. Fix this by using the
> right variable, maxBuffers.
> 
> Rename the misleadingly named count variable to avoid confusion in the
> future.
> 
> Fixes: 33fedea818e2b6a9 ("libcamera: pipeline_handler: Fold buffer management with start/stop")
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 97bb4f72cde5423e..d9a221d2df3e4b4c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -668,14 +668,14 @@ int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,
>  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	unsigned int count = 1;
> +	unsigned int ipaBufferIDCounter = 1;

That's a bit of a long name. How about bufferId ? Or ipaBufferId if you
really insist ? :-) (ID should be spelled Id in any case)

>  	int ret;
>  
>  	unsigned int maxBuffers = 0;
>  	for (const Stream *s : camera->streams())
>  		maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>  
> -	ret = video_->importBuffers(count);
> +	ret = video_->importBuffers(maxBuffers);

So before the faulty patch, we were importing, for each stream, the
number of buffers specified for that stream. Now we're importing the
maximum number of buffers. As the pipeline handler supports a single
stream this is equivalent, but is it the right thing to do when we'll
have multiple streams ?

>  	if (ret < 0)
>  		goto error;
>  
> @@ -688,14 +688,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>  		goto error;
>  
>  	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> -		buffer->setCookie(count++);
> +		buffer->setCookie(ipaBufferIDCounter++);
>  		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
>  					      .planes = buffer->planes() });
>  		availableParamBuffers_.push(buffer.get());
>  	}
>  
>  	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> -		buffer->setCookie(count++);
> +		buffer->setCookie(ipaBufferIDCounter++);
>  		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
>  					      .planes = buffer->planes() });
>  		availableStatBuffers_.push(buffer.get());

-- 
Regards,

Laurent Pinchart