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

[Jacopo Mondi]

Hi Laurent, Niklas,
  replying to a comment you both had.

On Fri, Apr 05, 2019 at 06:45:01PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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?

Thanks
   j

>
> > +
> >  	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
-------------- 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/20190408/254238dd/attachment-0001.sig>