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.
> +
> 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