| Message ID | 20191126233620.1695316-9-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Wed, Nov 27, 2019 at 12:35:58AM +0100, Niklas Söderlund wrote: > Dmabug and Plane serves similar functions in the two buffer interfaces In retrospect naming the API dmabuf was a big mistake, with f being so close to g on keyboards. Even Dvorak doesn't help. > Buffer and FrameBuffer and share many function signatures. Replace all > usages of Plane with Dmabuf to prepare for dropping the Buffer > interface. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/buffer.h | 6 +++--- > src/cam/buffer_writer.cpp | 6 +++--- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++-- > src/libcamera/stream.cpp | 3 +-- > src/libcamera/v4l2_videodevice.cpp | 14 ++++++-------- > src/qcam/main_window.cpp | 4 ++-- > test/camera/buffer_import.cpp | 2 +- > 7 files changed, 18 insertions(+), 21 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index fe5195327b540f5c..2dd5bcf3b49c4ee8 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -101,11 +101,11 @@ private: > class BufferMemory final > { > public: > - const std::vector<Plane> &planes() const { return planes_; } > - std::vector<Plane> &planes() { return planes_; } > + const std::vector<Dmabuf> &planes() const { return planes_; } > + std::vector<Dmabuf> &planes() { return planes_; } The BufferMemory class documentation mentions the Plane class, it should be replaced with Dmabuf there too. It's no big deal given that the BufferMemory class is dropped later, but you may still want to fix that. > > private: > - std::vector<Plane> planes_; > + std::vector<Dmabuf> planes_; > }; > > class BufferPool final > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp > index c33e99c5f8173db8..5967efca07254666 100644 > --- a/src/cam/buffer_writer.cpp > +++ b/src/cam/buffer_writer.cpp > @@ -43,9 +43,9 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName) > return -errno; > > BufferMemory *mem = buffer->mem(); > - for (Plane &plane : mem->planes()) { > - void *data = plane.mem(); > - unsigned int length = plane.length(); > + for (Dmabuf &dmabuf : mem->planes()) { > + void *data = dmabuf.mem(); > + unsigned int length = dmabuf.length(); > > ret = ::write(fd, data, length); > if (ret < 0) { > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 00fa4d4f19cb4aa4..e8b6a278e97b0ba0 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -689,7 +689,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, > for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) { > std::vector<FrameBuffer::Plane> planes; > planes.push_back({ > - .fd = paramPool_.buffers()[i].planes()[0].dmabuf(), > + .fd = paramPool_.buffers()[i].planes()[0].fd(), I like how .fd = <...>.fd(), it's nicely consistent. It's one of those small details that show we're getting it right. > .length = paramPool_.buffers()[i].planes()[0].length(), > }); > > @@ -701,7 +701,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, > for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) { > std::vector<FrameBuffer::Plane> planes; > planes.push_back({ > - .fd = statPool_.buffers()[i].planes()[0].dmabuf(), > + .fd = statPool_.buffers()[i].planes()[0].fd(), > .length = statPool_.buffers()[i].planes()[0].length(), > }); > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index 45f31ae1e2daeb53..e70a1e307ecaa5ba 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -577,8 +577,7 @@ int Stream::mapBuffer(const Buffer *buffer) > if (dmabufs[i] == -1) > break; > > - mem->planes().emplace_back(); > - mem->planes().back().setDmabuf(dmabufs[i], 0); > + mem->planes().emplace_back(dmabufs[i], 0); This, on the other hand, is less consistent, but the BufferMemory class is going away :-) With the BufferMemory documentation updated if desired, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > /* Remove the buffer from the cache and return its index. */ > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index dbb5c3982334e243..166b0abc1b101f88 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -921,9 +921,7 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index, > return ret; > } > > - buffer->planes().emplace_back(); > - Plane &plane = buffer->planes().back(); > - plane.setDmabuf(expbuf.fd, length); > + buffer->planes().emplace_back(expbuf.fd, length); > ::close(expbuf.fd); > > return 0; > @@ -986,19 +984,19 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer) > > bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type); > BufferMemory *mem = &bufferPool_->buffers()[buf.index]; > - const std::vector<Plane> &planes = mem->planes(); > + const std::vector<Dmabuf> &dmabufs = mem->planes(); > > if (buf.memory == V4L2_MEMORY_DMABUF) { > if (multiPlanar) { > - for (unsigned int p = 0; p < planes.size(); ++p) > - v4l2Planes[p].m.fd = planes[p].dmabuf(); > + for (unsigned int p = 0; p < dmabufs.size(); ++p) > + v4l2Planes[p].m.fd = dmabufs[p].fd(); > } else { > - buf.m.fd = planes[0].dmabuf(); > + buf.m.fd = dmabufs[0].fd(); > } > } > > if (multiPlanar) { > - buf.length = planes.size(); > + buf.length = dmabufs.size(); > buf.m.planes = v4l2Planes; > } > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 0c7ca61ac12ec41c..a828a176cbbf4854 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -300,8 +300,8 @@ int MainWindow::display(Buffer *buffer) > if (mem->planes().size() != 1) > return -EINVAL; > > - Plane &plane = mem->planes().front(); > - unsigned char *raw = static_cast<unsigned char *>(plane.mem()); > + Dmabuf &dmabuf = mem->planes().front(); > + unsigned char *raw = static_cast<unsigned char *>(dmabuf.mem()); > viewfinder_->display(raw, buffer->bytesused()); > > return 0; > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp > index 3efe02704c02f691..8b0d1c167bd2bbe8 100644 > --- a/test/camera/buffer_import.cpp > +++ b/test/camera/buffer_import.cpp > @@ -178,7 +178,7 @@ private: > > uint64_t cookie = index; > BufferMemory &mem = pool_.buffers()[index]; > - int dmabuf = mem.planes()[0].dmabuf(); > + int dmabuf = mem.planes()[0].fd(); > > requestReady.emit(cookie, dmabuf); >
diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h index fe5195327b540f5c..2dd5bcf3b49c4ee8 100644 --- a/include/libcamera/buffer.h +++ b/include/libcamera/buffer.h @@ -101,11 +101,11 @@ private: class BufferMemory final { public: - const std::vector<Plane> &planes() const { return planes_; } - std::vector<Plane> &planes() { return planes_; } + const std::vector<Dmabuf> &planes() const { return planes_; } + std::vector<Dmabuf> &planes() { return planes_; } private: - std::vector<Plane> planes_; + std::vector<Dmabuf> planes_; }; class BufferPool final diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp index c33e99c5f8173db8..5967efca07254666 100644 --- a/src/cam/buffer_writer.cpp +++ b/src/cam/buffer_writer.cpp @@ -43,9 +43,9 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName) return -errno; BufferMemory *mem = buffer->mem(); - for (Plane &plane : mem->planes()) { - void *data = plane.mem(); - unsigned int length = plane.length(); + for (Dmabuf &dmabuf : mem->planes()) { + void *data = dmabuf.mem(); + unsigned int length = dmabuf.length(); ret = ::write(fd, data, length); if (ret < 0) { diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 00fa4d4f19cb4aa4..e8b6a278e97b0ba0 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -689,7 +689,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) { std::vector<FrameBuffer::Plane> planes; planes.push_back({ - .fd = paramPool_.buffers()[i].planes()[0].dmabuf(), + .fd = paramPool_.buffers()[i].planes()[0].fd(), .length = paramPool_.buffers()[i].planes()[0].length(), }); @@ -701,7 +701,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) { std::vector<FrameBuffer::Plane> planes; planes.push_back({ - .fd = statPool_.buffers()[i].planes()[0].dmabuf(), + .fd = statPool_.buffers()[i].planes()[0].fd(), .length = statPool_.buffers()[i].planes()[0].length(), }); diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index 45f31ae1e2daeb53..e70a1e307ecaa5ba 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -577,8 +577,7 @@ int Stream::mapBuffer(const Buffer *buffer) if (dmabufs[i] == -1) break; - mem->planes().emplace_back(); - mem->planes().back().setDmabuf(dmabufs[i], 0); + mem->planes().emplace_back(dmabufs[i], 0); } /* Remove the buffer from the cache and return its index. */ diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index dbb5c3982334e243..166b0abc1b101f88 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -921,9 +921,7 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index, return ret; } - buffer->planes().emplace_back(); - Plane &plane = buffer->planes().back(); - plane.setDmabuf(expbuf.fd, length); + buffer->planes().emplace_back(expbuf.fd, length); ::close(expbuf.fd); return 0; @@ -986,19 +984,19 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer) bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type); BufferMemory *mem = &bufferPool_->buffers()[buf.index]; - const std::vector<Plane> &planes = mem->planes(); + const std::vector<Dmabuf> &dmabufs = mem->planes(); if (buf.memory == V4L2_MEMORY_DMABUF) { if (multiPlanar) { - for (unsigned int p = 0; p < planes.size(); ++p) - v4l2Planes[p].m.fd = planes[p].dmabuf(); + for (unsigned int p = 0; p < dmabufs.size(); ++p) + v4l2Planes[p].m.fd = dmabufs[p].fd(); } else { - buf.m.fd = planes[0].dmabuf(); + buf.m.fd = dmabufs[0].fd(); } } if (multiPlanar) { - buf.length = planes.size(); + buf.length = dmabufs.size(); buf.m.planes = v4l2Planes; } diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 0c7ca61ac12ec41c..a828a176cbbf4854 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -300,8 +300,8 @@ int MainWindow::display(Buffer *buffer) if (mem->planes().size() != 1) return -EINVAL; - Plane &plane = mem->planes().front(); - unsigned char *raw = static_cast<unsigned char *>(plane.mem()); + Dmabuf &dmabuf = mem->planes().front(); + unsigned char *raw = static_cast<unsigned char *>(dmabuf.mem()); viewfinder_->display(raw, buffer->bytesused()); return 0; diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp index 3efe02704c02f691..8b0d1c167bd2bbe8 100644 --- a/test/camera/buffer_import.cpp +++ b/test/camera/buffer_import.cpp @@ -178,7 +178,7 @@ private: uint64_t cookie = index; BufferMemory &mem = pool_.buffers()[index]; - int dmabuf = mem.planes()[0].dmabuf(); + int dmabuf = mem.planes()[0].fd(); requestReady.emit(cookie, dmabuf);
Dmabug and Plane serves similar functions in the two buffer interfaces Buffer and FrameBuffer and share many function signatures. Replace all usages of Plane with Dmabuf to prepare for dropping the Buffer interface. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/buffer.h | 6 +++--- src/cam/buffer_writer.cpp | 6 +++--- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++-- src/libcamera/stream.cpp | 3 +-- src/libcamera/v4l2_videodevice.cpp | 14 ++++++-------- src/qcam/main_window.cpp | 4 ++-- test/camera/buffer_import.cpp | 2 +- 7 files changed, 18 insertions(+), 21 deletions(-)