[libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple stream memory management
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 9 20:21:42 CEST 2019
Hi Laurent,
On Tue, Apr 09, 2019 at 05:06:39PM +0300, Laurent Pinchart wrote:
> 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.
>
Is this the part that concerns you?
from: Documentation/output/media/uapi/v4l/vidioc-reqbufs.html
Note that if any buffers are still mapped or exported via DMABUF,
then ioctl VIDIOC_REQBUFS can only succeed if the
V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS capability is set. Otherwise
ioctl VIDIOC_REQBUFS will return the EBUSY error code.
As I read this, and if that's you concern, we should close and unmap
any exported buffers before calling reqbufs(0).
I think that if allocation fails (in particular if exportBuffers() fails)
then the V4L2Device destroy all the buffer planes, closing any open fd
and unmapping them. Then we can freeBuffers in the driver memory
through freeBuffers. For non-yet-allocated buffers, they would receive
a reqbufs(0) ioctl, before any fd has been mapped or exported. It
seems ok to me, have I missed 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/20190409/7764d9c9/attachment.sig>
More information about the libcamera-devel
mailing list