[libcamera-devel] [PATCH v2 03/25] ipa: Switch to FrameBuffer interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 8 04:04:37 CET 2020
Hi Niklas,
One more thing.
On Tue, Jan 07, 2020 at 05:01:08AM +0200, Laurent Pinchart wrote:
> On Mon, Dec 30, 2019 at 01:04:48PM +0100, Niklas Söderlund wrote:
> > Switch the IPA interfaces and implementations to use the Framebuffer
> > interface.
> >
> > - The IPA interface is switch to use the simpler FrameBuffer::Plane
>
> s/switch/switched/
>
> > container when carrying dmabuf descriptions (fd and length) over the
> > pipeline/IPA boundary.
> >
> > - The RkISP1 IPA implementation takes advantage of the new simpler and
> > safer (better control over file descriptors) FrameBuffer interface.
> >
> > The biggest change can be observed in the test case where it is
> > demonstrated that FrameBuffer objects only carry the necessary number of
> > planes and not a fixed number. As a consequence of this the test needs
> > to verify the file descriptors it injects and not depend on the library
> > to handle uninitialized fields.
>
> Really ? It seems you set the number of planes to 3 and only initialize
> some of them. Did I miss something ?
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/ipa/ipa_interface.h | 2 +-
> > src/ipa/libipa/ipa_interface_wrapper.cpp | 9 ++----
> > src/ipa/rkisp1/rkisp1.cpp | 30 +++++++++++++----
> > src/libcamera/ipa_context_wrapper.cpp | 8 ++---
> > src/libcamera/ipa_interface.cpp | 7 ++--
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++--
> > test/ipa/ipa_wrappers_test.cpp | 41 +++++++++++++-----------
> > 7 files changed, 66 insertions(+), 43 deletions(-)
> >
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index 0ea53d120fe10acf..229d1124b19ec1c9 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -105,7 +105,7 @@ struct IPAStream {
> >
> > struct IPABuffer {
> > unsigned int id;
> > - BufferMemory memory;
> > + std::vector<FrameBuffer::Plane> planes;
>
> I wonder if we have a file descriptor lifetime management issue here.
> The IPA API doesn't define ownership for file descriptors :-S I think we
> need to address this first (to decide if the mapBuffers() method should
> be given ownership of the passed file descriptors or if it should borrow
> them), and then revisit this patch (with Jacopo's comments on 02/25
> taken into account). Should this then become a vector of unique_ptr, or
> a vector of const references ?
>
> > };
> >
> > struct IPAOperationData {
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index 6a389dfa714ab535..3628a785dc3e63f4 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -144,17 +144,14 @@ void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > for (unsigned int i = 0; i < num_buffers; ++i) {
> > const struct ipa_buffer &_buffer = _buffers[i];
> > IPABuffer &buffer = buffers[i];
> > - std::vector<Plane> &planes = buffer.memory.planes();
> > + std::vector<FrameBuffer::Plane> &planes = buffer.planes;
> >
> > buffer.id = _buffer.id;
> >
> > planes.resize(_buffer.num_planes);
> > for (unsigned int j = 0; j < _buffer.num_planes; ++j) {
> > - if (_buffer.planes[j].dmabuf != -1)
> > - planes[j].setDmabuf(_buffer.planes[j].dmabuf,
> > - _buffer.planes[j].length);
> > - /** \todo Create a Dmabuf class to implement RAII. */
> > - ::close(_buffer.planes[j].dmabuf);
> > + planes[j].fd = FileDescriptor(_buffer.planes[j].dmabuf);
> > + planes[j].length = _buffer.planes[j].length;
> > }
> > }
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 744a16ae5b9aa420..63776563b149f7ed 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -10,6 +10,7 @@
> > #include <queue>
> > #include <stdint.h>
> > #include <string.h>
> > +#include <sys/mman.h>
> >
> > #include <linux/rkisp1-config.h>
> >
> > @@ -48,7 +49,8 @@ private:
> > void setControls(unsigned int frame);
> > void metadataReady(unsigned int frame, unsigned int aeState);
> >
> > - std::map<unsigned int, BufferMemory> bufferInfo_;
> > + std::map<unsigned int, FrameBuffer> buffers_;
> > + std::map<unsigned int, void *> buffersMemory_;
> >
> > ControlInfoMap ctrls_;
> >
> > @@ -102,15 +104,29 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > {
> > for (const IPABuffer &buffer : buffers) {
> > - bufferInfo_[buffer.id] = buffer.memory;
> > - bufferInfo_[buffer.id].planes()[0].mem();
> > + buffers_.emplace(buffer.id, FrameBuffer(buffer.planes));
/* \todo Provide a helper to mmap() buffers (possibly exposed to applications) */
> > + buffersMemory_[buffer.id] = mmap(NULL,
> > + buffers_[buffer.id].planes()[0].length,
> > + PROT_READ | PROT_WRITE,
> > + MAP_SHARED,
> > + buffers_[buffer.id].planes()[0].fd.fd(),
> > + 0);
> > +
> > + if (buffersMemory_[buffer.id] == MAP_FAILED) {
> > + int ret = -errno;
> > + LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: "
> > + << strerror(-ret);
> > + }
> > }
> > }
> >
> > void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> > {
> > - for (unsigned int id : ids)
> > - bufferInfo_.erase(id);
> > + for (unsigned int id : ids) {
> > + munmap(buffersMemory_[id], buffers_[id].planes()[0].length);
> > + buffersMemory_.erase(id);
> > + buffers_.erase(id);
> > + }
> > }
> >
> > void IPARkISP1::processEvent(const IPAOperationData &event)
> > @@ -121,7 +137,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)
> > unsigned int bufferId = event.data[1];
> >
> > const rkisp1_stat_buffer *stats =
> > - static_cast<rkisp1_stat_buffer *>(bufferInfo_[bufferId].planes()[0].mem());
> > + static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]);
> >
> > updateStatistics(frame, stats);
> > break;
> > @@ -131,7 +147,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)
> > unsigned int bufferId = event.data[1];
> >
> > rkisp1_isp_params_cfg *params =
> > - static_cast<rkisp1_isp_params_cfg *>(bufferInfo_[bufferId].planes()[0].mem());
> > + static_cast<rkisp1_isp_params_cfg *>(buffersMemory_[bufferId]);
> >
> > queueRequest(frame, params, event.controls[0]);
> > break;
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > index 9603fdac87150aa0..946a2fd8cab1782a 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -149,15 +149,15 @@ void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
> > for (unsigned int i = 0; i < buffers.size(); ++i) {
> > struct ipa_buffer &c_buffer = c_buffers[i];
> > const IPABuffer &buffer = buffers[i];
> > - const std::vector<Plane> &planes = buffer.memory.planes();
> > + const std::vector<FrameBuffer::Plane> &planes = buffer.planes;
> >
> > c_buffer.id = buffer.id;
> > c_buffer.num_planes = planes.size();
> >
> > for (unsigned int j = 0; j < planes.size(); ++j) {
> > - const Plane &plane = planes[j];
> > - c_buffer.planes[j].dmabuf = plane.dmabuf();
> > - c_buffer.planes[j].length = plane.length();
> > + const FrameBuffer::Plane &plane = planes[j];
> > + c_buffer.planes[j].dmabuf = plane.fd.fd();
> > + c_buffer.planes[j].length = plane.length;
> > }
> > }
> >
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index ee3e3622f39ae85f..2f86087ee4aa414f 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -334,11 +334,10 @@ namespace libcamera {
> > */
> >
> > /**
> > - * \var IPABuffer::memory
> > - * \brief The buffer memory description
> > + * \var IPABuffer::planes
> > + * \brief The buffer planes description
> > *
> > - * The memory field stores the dmabuf handle and size for each plane of the
> > - * buffer.
> > + * Stores the dmabuf handle and length for each plane of the buffer.
> > */
> >
> > /**
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 7e41222e3d01a475..d7ee95ded0f76027 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -687,14 +687,22 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> > }
> >
> > for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> > + FrameBuffer::Plane plane;
> > + plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());
> > + plane.length = paramPool_.buffers()[i].planes()[0].length();
> > +
> > data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
> > - .memory = paramPool_.buffers()[i] });
> > + .planes = { plane } });
> > paramBuffers_.push(new Buffer(i));
> > }
> >
> > for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> > + FrameBuffer::Plane plane;
> > + plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());
> > + plane.length = statPool_.buffers()[i].planes()[0].length();
> > +
> > data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
> > - .memory = statPool_.buffers()[i] });
> > + .planes = { plane } });
> > statBuffers_.push(new Buffer(i));
> > }
> >
> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > index a1e34ad52317f47b..e711e4fe318d8179 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -115,28 +115,28 @@ public:
> > return report(Op_mapBuffers, TestFail);
> > }
> >
> > - if (buffers[0].memory.planes().size() != 3 ||
> > - buffers[1].memory.planes().size() != 3) {
> > + if (buffers[0].planes.size() != 3 ||
> > + buffers[1].planes.size() != 3) {
> > cerr << "mapBuffers(): Invalid number of planes" << endl;
> > return report(Op_mapBuffers, TestFail);
> > }
> >
> > - if (buffers[0].memory.planes()[0].length() != 4096 ||
> > - buffers[0].memory.planes()[1].length() != 0 ||
> > - buffers[0].memory.planes()[2].length() != 0 ||
> > - buffers[0].memory.planes()[0].length() != 4096 ||
> > - buffers[1].memory.planes()[1].length() != 4096 ||
> > - buffers[1].memory.planes()[2].length() != 0) {
> > + if (buffers[0].planes[0].length != 4096 ||
> > + buffers[0].planes[1].length != 0 ||
> > + buffers[0].planes[2].length != 0 ||
> > + buffers[0].planes[0].length != 4096 ||
> > + buffers[1].planes[1].length != 4096 ||
> > + buffers[1].planes[2].length != 0) {
> > cerr << "mapBuffers(): Invalid length" << endl;
> > return report(Op_mapBuffers, TestFail);
> > }
> >
> > - if (buffers[0].memory.planes()[0].dmabuf() == -1 ||
> > - buffers[0].memory.planes()[1].dmabuf() != -1 ||
> > - buffers[0].memory.planes()[2].dmabuf() != -1 ||
> > - buffers[0].memory.planes()[0].dmabuf() == -1 ||
> > - buffers[1].memory.planes()[1].dmabuf() == -1 ||
> > - buffers[1].memory.planes()[2].dmabuf() != -1) {
> > + if (buffers[0].planes[0].fd.fd() == -1 ||
> > + buffers[0].planes[1].fd.fd() != -1 ||
> > + buffers[0].planes[2].fd.fd() != -1 ||
> > + buffers[0].planes[0].fd.fd() == -1 ||
> > + buffers[1].planes[1].fd.fd() == -1 ||
> > + buffers[1].planes[2].fd.fd() != -1) {
> > cerr << "mapBuffers(): Invalid dmabuf" << endl;
> > return report(Op_mapBuffers, TestFail);
> > }
> > @@ -287,13 +287,16 @@ protected:
> >
> > /* Test mapBuffers(). */
> > std::vector<IPABuffer> buffers(2);
> > - buffers[0].memory.planes().resize(3);
> > + buffers[0].planes.resize(3);
> > buffers[0].id = 10;
> > - buffers[0].memory.planes()[0].setDmabuf(fd_, 4096);
> > + buffers[0].planes[0].fd = FileDescriptor(fd_);
> > + buffers[0].planes[0].length = 4096;
> > buffers[1].id = 11;
> > - buffers[1].memory.planes().resize(3);
> > - buffers[1].memory.planes()[0].setDmabuf(fd_, 4096);
> > - buffers[1].memory.planes()[1].setDmabuf(fd_, 4096);
> > + buffers[1].planes.resize(3);
> > + buffers[1].planes[0].fd = FileDescriptor(fd_);
> > + buffers[1].planes[0].length = 4096;
> > + buffers[1].planes[1].fd = FileDescriptor(fd_);
> > + buffers[1].planes[1].length = 4096;
> >
> > ret = INVOKE(mapBuffers, buffers);
> > if (ret == TestFail)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list