[libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple stream memory management

[Laurent Pinchart]

Hi Jacopo,

On Mon, Apr 08, 2019 at 05:50:28PM +0200, Jacopo Mondi wrote:
> On Fri, Apr 05, 2019 at 06:45:01PM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 03, 2019 at 05:07:31PM +0200, Jacopo Mondi wrote:
> >> Perform allocation and setup of memory sharing betweent the CIO2 output
> >
> > s/betweent/between/
> >
> >> and the ImgU input and allocate memory for each active stream.
> >> ---
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------
> >>  1 file changed, 62 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index a12e3f9a496d..f7e75fac1dfe 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -91,6 +91,7 @@ public:
> >>
> >>  	BufferPool vfPool_;
> >>  	BufferPool statPool_;
> >> +	BufferPool outPool_;
> >>  };
> >>
> >>  class CIO2Device
> >> @@ -428,13 +429,21 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> >>  	return 0;
> >>  }
> >>
> >> +/**
> >> + * \todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and
> >> + * started even if not in use. As of now, if not properly configured and
> >> + * enabled, the ImgU processing pipeline stalls.
> >> + *
> >> + * In order to be able to start the 'viewfinder' and 'stat' nodes, we need
> >> + * memory to be reserved.
> >> + */
> >>  int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> >>  					 const std::set<Stream *> &streams)
> >>  {
> >>  	IPU3CameraData *data = cameraData(camera);
> >> -	Stream *stream = *streams.begin();
> >>  	CIO2Device *cio2 = &data->cio2_;
> >>  	ImgUDevice *imgu = data->imgu_;
> >> +	unsigned int bufferCount;
> >>  	int ret;
> >>
> >>  	/* Share buffers between CIO2 output and ImgU input. */
> >> @@ -446,28 +455,64 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> >>  	if (ret)
> >>  		return ret;
> >>
> >> -	/* Export ImgU output buffers to the stream's pool. */
> >> -	ret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());
> >> -	if (ret)
> >> -		return ret;
> >> -
> >>  	/*
> >> -	 * Reserve memory in viewfinder and stat output devices. Use the
> >> -	 * same number of buffers as the ones requested for the output
> >> -	 * stream.
> >> +	 * Use for internal pools the same buffer number as the input
> >
> > s/buffer number/number of buffers/
> > s/as the input one/as for the input pool/
> >
> >> +	 * one: it should match the pipeline lenght.
> >
> > s/lenght/length/
> >
> > And what do you mean by pipeline length here ?
> >
> >>  	 */
> >> -	unsigned int bufferCount = stream->bufferPool().count();
> >> -
> >> -	imgu->viewfinder_.pool->createBuffers(bufferCount);
> >> -	ret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >> +	bufferCount = pool->count();
> >
> > I think you can keep the variable declaration here, there's no need to
> > move it to the beginning of the function.
> >
> >>  	imgu->stat_.pool->createBuffers(bufferCount);
> >>  	ret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);
> >>  	if (ret)
> >>  		return ret;
> >>
> >> +	for (Stream *stream : streams) {
> >> +		if (isOutput(data, stream)) {
> >> +			/* Export output buffers to the stream's pool. */
> >> +			ret = imgu->exportBuffers(&imgu->output_,
> >> +						  &stream->bufferPool());
> >> +			if (ret)
> >> +				return ret;
> >> +
> >> +			if (isViewfinderActive(data))
> >> +				continue;
> >> +
> >> +			/*
> >> +			 * Reserve memory in viewfinder device if it is not
> >> +			 * part of the active streams list. Use the same
> >> +			 * number of buffers as the ones requested for the
> >> +			 * output stream.
> >> +			 */
> >> +			bufferCount = stream->bufferPool().count();
> >> +			imgu->viewfinder_.pool->createBuffers(bufferCount);
> >> +			ret = imgu->exportBuffers(&imgu->viewfinder_,
> >> +						  imgu->viewfinder_.pool);
> >> +			if (ret)
> >> +				return ret;
> >> +		} else if (isViewfinder(data, stream)) {
> >> +			/* Export viewfinder buffers to the stream's pool. */
> >> +			ret = imgu->exportBuffers(&imgu->viewfinder_,
> >> +						  &stream->bufferPool());
> >> +			if (ret)
> >> +				return ret;
> >> +
> >> +			if (isOutputActive(data))
> >> +				continue;
> >> +
> >> +			/*
> >> +			 * Reserve memory in output device if it is not part
> >> +			 * of the active streams list. Use the same number
> >> +			 * of buffers as the ones requested for the viewfinder
> >> +			 * stream.
> >> +			 */
> >> +			bufferCount = stream->bufferPool().count();
> >> +			imgu->output_.pool->createBuffers(bufferCount);
> >> +			ret = imgu->exportBuffers(&imgu->output_,
> >> +						  imgu->output_.pool);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> >> +	}
> >
> > As for the previous patch, I think you should allocate memory for the
> > streams present in the set in a loop without caring about the stream
> > type, and then for the other devices outside of the loop. It should also
> > ease error handling, which is missing here.
> 
> Both you and Niklas reported that errors are not handled by this
> function. Actually, if allocateBuffers fail, the Camera class calls
> freeBuffers, which will free the allocated one and will simply call
> reqBuffers(0) on the non allocated one. In my opinion this seems safe,
> have I mis-interpreted something?

Do you think it's safe to call the freeBuffers() function if
allocateBuffers() failed ? That would require handling partial
allocation in freeBuffers(). I think it would be best to clean up on
failure in allocateBuffers(), and not call freeBuffers() in that case.
The code to handle failure after partial allocation will then be close
to the location of the failure, I think that would be less error-prone.

> >> +
> >>  	return 0;
> >>  }
> >>
> >> @@ -819,6 +864,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
> >>
> >>  	output_.pad = PAD_OUTPUT;
> >>  	output_.name = "output";
> >> +	output_.pool = &outPool_;
> >>
> >>  	viewfinder_.dev = V4L2Device::fromEntityName(media,
> >>  						     name_ + " viewfinder");

-- 
Regards,

Laurent Pinchart