| Message ID | 20190403150735.27580-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-04-03 17:07:31 +0200, Jacopo Mondi wrote: > Perform allocation and setup of memory sharing betweent the CIO2 output > 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 > + * one: it should match the pipeline lenght. > */ > - 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(); > 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; > + } > + } > + This change looks ok to me but I wonder about the error path here. What happens if exportBuffers fails for the second stream? I'm only curious :-) > 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"); > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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");
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
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");
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
Hi Jacopo, On Tue, Apr 09, 2019 at 08:21:42PM +0200, Jacopo Mondi wrote: > On Tue, Apr 09, 2019 at 05:06:39PM +0300, Laurent Pinchart wrote: > > 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). No, what concerns me is internal to libcamera. It's an API matter, how to deal with cleanups after partial failure. There are two models, requiring functions to cleanup internally when they return a failure, or calling the cleanup function on failure. In the first case the cleanup function only has to deal with fully initialised states, while in the second case the cleanup function has to deal with partially initialised states if a partial failure occurs. I think I prefer the former as it groups the partial failure handling with the code where partial failure occurs. > 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? > > > > >> + > > > >> 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");
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 + * one: it should match the pipeline lenght. */ - 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(); 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; + } + } + 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");