Message ID | 20191126233620.1695316-20-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, On Wed, Nov 27, 2019 at 12:36:09AM +0100, Niklas Söderlund wrote: > Extend V4L2VideoDevice with two new functions, one to deal with > allocating buffers from the video device and one to prepare the video > device to use external buffers. > > The two new functions intend to replace exportBuffers() and > importBuffers() once the transition to the FrameBuffer interface is > complete. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/include/v4l2_videodevice.h | 4 + > src/libcamera/v4l2_videodevice.cpp | 126 ++++++++++++++++++++++- > 2 files changed, 129 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h > index 254f8797af42dd8a..f4cbcfbd26ea540c 100644 > --- a/src/libcamera/include/v4l2_videodevice.h > +++ b/src/libcamera/include/v4l2_videodevice.h > @@ -162,6 +162,8 @@ public: > > int exportBuffers(BufferPool *pool); > int importBuffers(BufferPool *pool); > + int allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers); > + int externalBuffers(unsigned int count); I agree with s/export/allocate, but why s/import/external ? Furthermore allocate/import is a nice combination of names > int releaseBuffers(); > > int queueBuffer(Buffer *buffer); > @@ -197,6 +199,7 @@ private: > int requestBuffers(unsigned int count); > int createPlane(BufferMemory *buffer, unsigned int index, > unsigned int plane, unsigned int length); > + FrameBuffer *createBuffer(struct v4l2_buffer buf); > int exportDmaBuffer(unsigned int index, unsigned int plane); > > Buffer *dequeueBuffer(); > @@ -208,6 +211,7 @@ private: > enum v4l2_memory memoryType_; > > BufferPool *bufferPool_; > + V4L2BufferCache *cache_; > std::map<unsigned int, Buffer *> queuedBuffers_; > > EventNotifier *fdEvent_; > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index c82f2829601bd14c..9fe66018ec502626 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -377,7 +377,8 @@ const std::string V4L2DeviceFormat::toString() const > * \param[in] deviceNode The file-system path to the video device node > */ > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > - : V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr) > + : V4L2Device(deviceNode), bufferPool_(nullptr), cache_(nullptr), > + fdEvent_(nullptr) > { > /* > * We default to an MMAP based CAPTURE video device, however this will > @@ -1042,6 +1043,100 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool) > return 0; > } > > +/** > + * \brief Operate using buffers allocated from local video device Allocate buffers from (in?) the video device > + * \param[in] count Number of buffers to allocate > + * \param[out] buffers Vector to store local buffers s/local// ? > + * \return 0 on success or a negative error code otherwise > + */ > +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers) > +{ > + unsigned int i; > + int ret; declare variables when you first use them > + > + if (cache_) { > + LOG(V4L2, Error) << "Can't allocate buffers when cache exists"; The presence of cache_ is an indication that memory has been allocated, not the sole reason of failure. I would LOG(V4L2, Error) << "Can't allocate buffers multiple times"; Or something similar > + return -EINVAL; > + } > + > + memoryType_ = V4L2_MEMORY_MMAP; > + > + ret = requestBuffers(count); > + if (ret < 0) > + return ret; > + > + for (i = 0; i < count; ++i) { Furthermore i is only used inside this loop > + struct v4l2_buffer buf = {}; > + struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; > + > + buf.index = i; > + buf.type = bufferType_; > + buf.memory = memoryType_; > + buf.length = VIDEO_MAX_PLANES; > + buf.m.planes = planes; > + > + ret = ioctl(VIDIOC_QUERYBUF, &buf); > + if (ret < 0) { ret = -errno; > + LOG(V4L2, Error) > + << "Unable to query buffer " << i << ": " > + << strerror(-ret); > + goto err_buf; > + } > + > + FrameBuffer *buffer = createBuffer(buf); > + if (!buffer) { > + LOG(V4L2, Error) << "Unable to create buffer"; > + ret = -EINVAL; > + goto err_buf; > + } > + > + buffers->push_back(buffer); We're dealing with pointers, so not a huge loss, but couldn't you provide the buffers vector to createBuffers and make that operation add the newly created buffer to the vector. In this way you could return an error code.. > + } > + > + cache_ = new V4L2BufferCache(*buffers); > + > + return count; empty line ? > +err_buf: > + requestBuffers(0); > + > + for (FrameBuffer *buffer : *buffers) > + delete buffer; > + > + buffers->clear(); > + > + return ret; > +} > + > +FrameBuffer *V4L2VideoDevice::createBuffer(struct v4l2_buffer buf) > +{ > + const unsigned int numPlanes = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? buf.length : 1; Can you break at 80 cols here? > + std::vector<FrameBuffer::Plane> planes; > + FrameBuffer *frame = nullptr; > + > + for (unsigned int nplane = 0; nplane < numPlanes; nplane++) { > + int fd = exportDmaBuffer(buf.index, nplane); > + if (fd < 0) > + goto out; > + > + FrameBuffer::Plane plane; > + plane.fd = fd; > + plane.length = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? > + buf.m.planes[nplane].length : buf.length; > + > + planes.emplace_back(plane); > + } > + > + if (!planes.empty()) > + frame = new FrameBuffer(planes); > + else > + LOG(V4L2, Error) << "Failed to get planes"; > +out: > + for (FrameBuffer::Plane &plane : planes) > + ::close(plane.fd); Can the plane fd be closed -before- creating the FrameBuffer ? If so you could Anway, I would return earlier if planes.empty() if (planes.empty()) { LOG(V4L2, Error) << "Failed to get planes"; return nullptr; } FrameBufffer *frame = new FrameBuffer(planes); for (FrameBuffer::Plane &plane : planes) ::close(plane.fd); return frame; Or if you provide buffers to this operation buffers.emaplce_back(planes); return 0; > + > + return frame; > +} > + > int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane) > { > struct v4l2_exportbuffer expbuf = {}; > @@ -1062,6 +1157,35 @@ int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane) > return expbuf.fd; > } > > +/** > + * \brief Operate using buffers imported from external source Import buffers externally allocated into the video device > + * \param[in] count Number of buffers to prepare for to prepare for/to import > + * \return 0 on success or a negative error code otherwise > + */ > +int V4L2VideoDevice::externalBuffers(unsigned int count) > +{ > + /* > + * We can only prepare the device to use external buffers when no > + * internal buffers have been allocated. > + */ > + if (cache_) > + return 0; I would log an error, as you do the same in allocate > + > + int ret; declare when use > + > + memoryType_ = V4L2_MEMORY_DMABUF; > + > + ret = requestBuffers(count); > + if (ret) > + return ret; > + > + cache_ = new V4L2BufferCache(count); > + > + LOG(V4L2, Debug) << "provided for " << count << " external buffers"; "provided for" sounds weird to me Thanks j > + > + return 0; > +} > + > /** > * \brief Release all internally allocated buffers > */ > -- > 2.24.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Mon, Dec 02, 2019 at 12:25:48PM +0100, Jacopo Mondi wrote: > On Wed, Nov 27, 2019 at 12:36:09AM +0100, Niklas Söderlund wrote: > > Extend V4L2VideoDevice with two new functions, one to deal with > > allocating buffers from the video device and one to prepare the video > > device to use external buffers. > > > > The two new functions intend to replace exportBuffers() and > > importBuffers() once the transition to the FrameBuffer interface is > > complete. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/include/v4l2_videodevice.h | 4 + > > src/libcamera/v4l2_videodevice.cpp | 126 ++++++++++++++++++++++- > > 2 files changed, 129 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h > > index 254f8797af42dd8a..f4cbcfbd26ea540c 100644 > > --- a/src/libcamera/include/v4l2_videodevice.h > > +++ b/src/libcamera/include/v4l2_videodevice.h > > @@ -162,6 +162,8 @@ public: > > > > int exportBuffers(BufferPool *pool); > > int importBuffers(BufferPool *pool); > > + int allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers); > > + int externalBuffers(unsigned int count); > > I agree with s/export/allocate, but why s/import/external ? > Furthermore allocate/import is a nice combination of names I agree with Jacopo here, externalBuffers() is a bad name. You could reuse the names exportBuffers() and importBuffers() as the arguments are different. importBuffers() isn't the best name ever though, as the method doesn't really import buffers, so feel free to propose better names. > > int releaseBuffers(); > > > > int queueBuffer(Buffer *buffer); > > @@ -197,6 +199,7 @@ private: > > int requestBuffers(unsigned int count); > > int createPlane(BufferMemory *buffer, unsigned int index, > > unsigned int plane, unsigned int length); > > + FrameBuffer *createBuffer(struct v4l2_buffer buf); const struct v4l2_buffer &buf It should have become an automatic reflex by now :-) > > int exportDmaBuffer(unsigned int index, unsigned int plane); > > > > Buffer *dequeueBuffer(); > > @@ -208,6 +211,7 @@ private: > > enum v4l2_memory memoryType_; > > > > BufferPool *bufferPool_; > > + V4L2BufferCache *cache_; > > std::map<unsigned int, Buffer *> queuedBuffers_; > > > > EventNotifier *fdEvent_; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index c82f2829601bd14c..9fe66018ec502626 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -377,7 +377,8 @@ const std::string V4L2DeviceFormat::toString() const > > * \param[in] deviceNode The file-system path to the video device node > > */ > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > > - : V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr) > > + : V4L2Device(deviceNode), bufferPool_(nullptr), cache_(nullptr), > > + fdEvent_(nullptr) > > { > > /* > > * We default to an MMAP based CAPTURE video device, however this will > > @@ -1042,6 +1043,100 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool) > > return 0; > > } > > > > +/** > > + * \brief Operate using buffers allocated from local video device > > Allocate buffers from (in?) the video device > > > + * \param[in] count Number of buffers to allocate > > + * \param[out] buffers Vector to store local buffers > > s/local// ? s/local/allocated/ > > + * \return 0 on success or a negative error code otherwise > > + */ > > +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers) I would wrap this to avoid long lines. > > +{ > > + unsigned int i; > > + int ret; > > declare variables when you first use them > > > + > > + if (cache_) { > > + LOG(V4L2, Error) << "Can't allocate buffers when cache exists"; > > The presence of cache_ is an indication that memory has been > allocated, not the sole reason of failure. I would > > LOG(V4L2, Error) << "Can't allocate buffers multiple times"; > > Or something similar "Buffers already allocated" ? > > + return -EINVAL; > > + } > > + > > + memoryType_ = V4L2_MEMORY_MMAP; > > + > > + ret = requestBuffers(count); > > + if (ret < 0) > > + return ret; > > + > > + for (i = 0; i < count; ++i) { > > Furthermore i is only used inside this loop for (unsigned int i = 0 ...) > > + struct v4l2_buffer buf = {}; > > + struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; > > + > > + buf.index = i; > > + buf.type = bufferType_; > > + buf.memory = memoryType_; > > + buf.length = VIDEO_MAX_PLANES; ARRAY_SIZE(planes) ? > > + buf.m.planes = planes; > > + > > + ret = ioctl(VIDIOC_QUERYBUF, &buf); > > + if (ret < 0) { > > ret = -errno; This is V4L2Device::ioctl(), not std::ioctl(), so the function returns the error code. > > + LOG(V4L2, Error) > > + << "Unable to query buffer " << i << ": " > > + << strerror(-ret); > > + goto err_buf; > > + } > > + > > + FrameBuffer *buffer = createBuffer(buf); > > + if (!buffer) { > > + LOG(V4L2, Error) << "Unable to create buffer"; > > + ret = -EINVAL; > > + goto err_buf; > > + } > > + > > + buffers->push_back(buffer); > > We're dealing with pointers, so not a huge loss, but couldn't you > provide the buffers vector to createBuffers and make that operation s/createBuffers/createBuffer/ > add the newly created buffer to the vector. In this way you could > return an error code.. I think it would make the interface of createBuffer() a bit more cryptic, but it's an internal API. I'm tempted to start using ERR_PTR() but I don't think it would be a good idea :-) > > + } > > + > > + cache_ = new V4L2BufferCache(*buffers); > > + > > + return count; > > empty line ? > > > +err_buf: > > + requestBuffers(0); > > + > > + for (FrameBuffer *buffer : *buffers) > > + delete buffer; > > + > > + buffers->clear(); > > + > > + return ret; > > +} > > + > > +FrameBuffer *V4L2VideoDevice::createBuffer(struct v4l2_buffer buf) How about calling this method createFrameBuffer() ? > > +{ > > + const unsigned int numPlanes = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? buf.length : 1; > > Can you break at 80 cols here? > > > + std::vector<FrameBuffer::Plane> planes; > > + FrameBuffer *frame = nullptr; s/frame/frameBuffer/ > > + > > + for (unsigned int nplane = 0; nplane < numPlanes; nplane++) { > > + int fd = exportDmaBuffer(buf.index, nplane); > > + if (fd < 0) > > + goto out; > > + > > + FrameBuffer::Plane plane; > > + plane.fd = fd; > > + plane.length = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? > > + buf.m.planes[nplane].length : buf.length; > > + > > + planes.emplace_back(plane); This won't save anything compared to push_back(). You should use planes.emplace_back(plane.fd, length); with a constructor for FrameBuffer::Plane, or use .push_back() to avoid giving the false impression that the code is optimized. > > + } > > + > > + if (!planes.empty()) > > + frame = new FrameBuffer(planes); > > + else > > + LOG(V4L2, Error) << "Failed to get planes"; This can only happen if the caller gives us a buf.length == 0. Let's move this to the beginning of the method with if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) { LOG(V4L2, Error) << "Invalid number of planes"; return nullptr; } (but maybe we trust the kernel and this could be dropped completely ?) and here just do frameBuffer = new FrameBuffer(planes); Missing empty line. > > +out: > > + for (FrameBuffer::Plane &plane : planes) > > + ::close(plane.fd); > > Can the plane fd be closed -before- creating the FrameBuffer ? > If so you could > > Anway, I would return earlier if planes.empty() > > if (planes.empty()) { > LOG(V4L2, Error) << "Failed to get planes"; > return nullptr; > } > > FrameBufffer *frame = new FrameBuffer(planes); > for (FrameBuffer::Plane &plane : planes) > ::close(plane.fd); > > return frame; > > Or if you provide buffers to this operation > buffers.emaplce_back(planes); > > return 0; And with proper use of the FileDescriptor class (with move semantics where appropriate), you should be able to drop the close() calls and return instead of goto out in the loop. > > + > > + return frame; > > +} > > + > > int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane) > > { > > struct v4l2_exportbuffer expbuf = {}; > > @@ -1062,6 +1157,35 @@ int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane) > > return expbuf.fd; > > } > > > > +/** > > + * \brief Operate using buffers imported from external source > > Import buffers externally allocated into the video device How about * \brief Prepare the device to import \a count buffers > > + * \param[in] count Number of buffers to prepare for > > to prepare for/to import > > > + * \return 0 on success or a negative error code otherwise > > + */ > > +int V4L2VideoDevice::externalBuffers(unsigned int count) > > +{ > > + /* > > + * We can only prepare the device to use external buffers when no > > + * internal buffers have been allocated. > > + */ Wrong indentation. > > + if (cache_) > > + return 0; > > I would log an error, as you do the same in allocate And return an error too. > > + > > + int ret; > > declare when use > > > + > > + memoryType_ = V4L2_MEMORY_DMABUF; > > + > > + ret = requestBuffers(count); > > + if (ret) > > + return ret; > > + > > + cache_ = new V4L2BufferCache(count); > > + > > + LOG(V4L2, Debug) << "provided for " << count << " external buffers"; > > "provided for" sounds weird to me Maybe "Prepared to use " << count << " external buffers"; or "Prepared to import " << count << " buffers"; > > + > > + return 0; > > +} > > + > > /** > > * \brief Release all internally allocated buffers > > */
diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h index 254f8797af42dd8a..f4cbcfbd26ea540c 100644 --- a/src/libcamera/include/v4l2_videodevice.h +++ b/src/libcamera/include/v4l2_videodevice.h @@ -162,6 +162,8 @@ public: int exportBuffers(BufferPool *pool); int importBuffers(BufferPool *pool); + int allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers); + int externalBuffers(unsigned int count); int releaseBuffers(); int queueBuffer(Buffer *buffer); @@ -197,6 +199,7 @@ private: int requestBuffers(unsigned int count); int createPlane(BufferMemory *buffer, unsigned int index, unsigned int plane, unsigned int length); + FrameBuffer *createBuffer(struct v4l2_buffer buf); int exportDmaBuffer(unsigned int index, unsigned int plane); Buffer *dequeueBuffer(); @@ -208,6 +211,7 @@ private: enum v4l2_memory memoryType_; BufferPool *bufferPool_; + V4L2BufferCache *cache_; std::map<unsigned int, Buffer *> queuedBuffers_; EventNotifier *fdEvent_; diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index c82f2829601bd14c..9fe66018ec502626 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -377,7 +377,8 @@ const std::string V4L2DeviceFormat::toString() const * \param[in] deviceNode The file-system path to the video device node */ V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) - : V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr) + : V4L2Device(deviceNode), bufferPool_(nullptr), cache_(nullptr), + fdEvent_(nullptr) { /* * We default to an MMAP based CAPTURE video device, however this will @@ -1042,6 +1043,100 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool) return 0; } +/** + * \brief Operate using buffers allocated from local video device + * \param[in] count Number of buffers to allocate + * \param[out] buffers Vector to store local buffers + * \return 0 on success or a negative error code otherwise + */ +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers) +{ + unsigned int i; + int ret; + + if (cache_) { + LOG(V4L2, Error) << "Can't allocate buffers when cache exists"; + return -EINVAL; + } + + memoryType_ = V4L2_MEMORY_MMAP; + + ret = requestBuffers(count); + if (ret < 0) + return ret; + + for (i = 0; i < count; ++i) { + struct v4l2_buffer buf = {}; + struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; + + buf.index = i; + buf.type = bufferType_; + buf.memory = memoryType_; + buf.length = VIDEO_MAX_PLANES; + buf.m.planes = planes; + + ret = ioctl(VIDIOC_QUERYBUF, &buf); + if (ret < 0) { + LOG(V4L2, Error) + << "Unable to query buffer " << i << ": " + << strerror(-ret); + goto err_buf; + } + + FrameBuffer *buffer = createBuffer(buf); + if (!buffer) { + LOG(V4L2, Error) << "Unable to create buffer"; + ret = -EINVAL; + goto err_buf; + } + + buffers->push_back(buffer); + } + + cache_ = new V4L2BufferCache(*buffers); + + return count; +err_buf: + requestBuffers(0); + + for (FrameBuffer *buffer : *buffers) + delete buffer; + + buffers->clear(); + + return ret; +} + +FrameBuffer *V4L2VideoDevice::createBuffer(struct v4l2_buffer buf) +{ + const unsigned int numPlanes = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? buf.length : 1; + std::vector<FrameBuffer::Plane> planes; + FrameBuffer *frame = nullptr; + + for (unsigned int nplane = 0; nplane < numPlanes; nplane++) { + int fd = exportDmaBuffer(buf.index, nplane); + if (fd < 0) + goto out; + + FrameBuffer::Plane plane; + plane.fd = fd; + plane.length = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? + buf.m.planes[nplane].length : buf.length; + + planes.emplace_back(plane); + } + + if (!planes.empty()) + frame = new FrameBuffer(planes); + else + LOG(V4L2, Error) << "Failed to get planes"; +out: + for (FrameBuffer::Plane &plane : planes) + ::close(plane.fd); + + return frame; +} + int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane) { struct v4l2_exportbuffer expbuf = {}; @@ -1062,6 +1157,35 @@ int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane) return expbuf.fd; } +/** + * \brief Operate using buffers imported from external source + * \param[in] count Number of buffers to prepare for + * \return 0 on success or a negative error code otherwise + */ +int V4L2VideoDevice::externalBuffers(unsigned int count) +{ + /* + * We can only prepare the device to use external buffers when no + * internal buffers have been allocated. + */ + if (cache_) + return 0; + + int ret; + + memoryType_ = V4L2_MEMORY_DMABUF; + + ret = requestBuffers(count); + if (ret) + return ret; + + cache_ = new V4L2BufferCache(count); + + LOG(V4L2, Debug) << "provided for " << count << " external buffers"; + + return 0; +} + /** * \brief Release all internally allocated buffers */
Extend V4L2VideoDevice with two new functions, one to deal with allocating buffers from the video device and one to prepare the video device to use external buffers. The two new functions intend to replace exportBuffers() and importBuffers() once the transition to the FrameBuffer interface is complete. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/include/v4l2_videodevice.h | 4 + src/libcamera/v4l2_videodevice.cpp | 126 ++++++++++++++++++++++- 2 files changed, 129 insertions(+), 1 deletion(-)