[{"id":3167,"web_url":"https://patchwork.libcamera.org/comment/3167/","msgid":"<20191202112548.jizi5hqkv65ou2le@uno.localdomain>","date":"2019-12-02T11:25:48","subject":"Re: [libcamera-devel] [PATCH 19/30] libcamera: v4l2_videodevice:\n\tAdd new buffer interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Nov 27, 2019 at 12:36:09AM +0100, Niklas Söderlund wrote:\n> Extend V4L2VideoDevice with two new functions, one to deal with\n> allocating buffers from the video device and one to prepare the video\n> device to use external buffers.\n>\n> The two new functions intend to replace exportBuffers() and\n> importBuffers() once the transition to the FrameBuffer interface is\n> complete.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/v4l2_videodevice.h | 4 +\n> src/libcamera/v4l2_videodevice.cpp | 126 ++++++++++++++++++++++-\n> 2 files changed, 129 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index 254f8797af42dd8a..f4cbcfbd26ea540c 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -162,6 +162,8 @@ public:\n>\n> \tint exportBuffers(BufferPool *pool);\n> \tint importBuffers(BufferPool *pool);\n> +\tint allocateBuffers(unsigned int count, std::vector *buffers);\n> +\tint externalBuffers(unsigned int count);\n\nI agree with s/export/allocate, but why s/import/external ?\nFurthermore allocate/import is a nice combination of names\n\n> \tint releaseBuffers();\n>\n> \tint queueBuffer(Buffer *buffer);\n> @@ -197,6 +199,7 @@ private:\n> \tint requestBuffers(unsigned int count);\n> \tint createPlane(BufferMemory *buffer, unsigned int index,\n> \t\t\tunsigned int plane, unsigned int length);\n> +\tFrameBuffer *createBuffer(struct v4l2_buffer buf);\n> \tint exportDmaBuffer(unsigned int index, unsigned int plane);\n>\n> \tBuffer *dequeueBuffer();\n> @@ -208,6 +211,7 @@ private:\n> \tenum v4l2_memory memoryType_;\n>\n> \tBufferPool *bufferPool_;\n> +\tV4L2BufferCache *cache_;\n> \tstd::map queuedBuffers_;\n>\n> \tEventNotifier *fdEvent_;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index c82f2829601bd14c..9fe66018ec502626 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -377,7 +377,8 @@ const std::string V4L2DeviceFormat::toString() const\n> * \\param[in] deviceNode The file-system path to the video device node\n> */\n> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> -\t: V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)\n> +\t: V4L2Device(deviceNode), bufferPool_(nullptr), cache_(nullptr),\n> +\t fdEvent_(nullptr)\n> {\n> \t/*\n> \t * We default to an MMAP based CAPTURE video device, however this will\n> @@ -1042,6 +1043,100 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool)\n> \treturn 0;\n> }\n>\n> +/**\n> + * \\brief Operate using buffers allocated from local video device\n\nAllocate buffers from (in?) the video device\n\n> + * \\param[in] count Number of buffers to allocate\n> + * \\param[out] buffers Vector to store local buffers\n\ns/local// ?\n\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector *buffers)\n> +{\n> +\tunsigned int i;\n> +\tint ret;\n\ndeclare variables when you first use them\n\n> +\n> +\tif (cache_) {\n> +\t\tLOG(V4L2, Error) << \"Can't allocate buffers when cache exists\";\n\nThe presence of cache_ is an indication that memory has been\nallocated, not the sole reason of failure. I would\n\n\t\tLOG(V4L2, Error) << \"Can't allocate buffers multiple times\";\n\nOr something similar\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tmemoryType_ = V4L2_MEMORY_MMAP;\n> +\n> +\tret = requestBuffers(count);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tfor (i = 0; i < count; ++i) {\n\nFurthermore i is only used inside this loop\n\n> +\t\tstruct v4l2_buffer buf = {};\n> +\t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> +\n> +\t\tbuf.index = i;\n> +\t\tbuf.type = bufferType_;\n> +\t\tbuf.memory = memoryType_;\n> +\t\tbuf.length = VIDEO_MAX_PLANES;\n> +\t\tbuf.m.planes = planes;\n> +\n> +\t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> +\t\tif (ret < 0) {\n\nret = -errno;\n\n> +\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> +\t\t\t\t<< strerror(-ret);\n> +\t\t\tgoto err_buf;\n> +\t\t}\n> +\n> +\t\tFrameBuffer *buffer = createBuffer(buf);\n> +\t\tif (!buffer) {\n> +\t\t\tLOG(V4L2, Error) << \"Unable to create buffer\";\n> +\t\t\tret = -EINVAL;\n> +\t\t\tgoto err_buf;\n> +\t\t}\n> +\n> +\t\tbuffers->push_back(buffer);\n\nWe're dealing with pointers, so not a huge loss, but couldn't you\nprovide the buffers vector to createBuffers and make that operation\nadd the newly created buffer to the vector. In this way you could\nreturn an error code..\n\n> +\t}\n> +\n> +\tcache_ = new V4L2BufferCache(*buffers);\n> +\n> +\treturn count;\n\nempty line ?\n\n> +err_buf:\n> +\trequestBuffers(0);\n> +\n> +\tfor (FrameBuffer *buffer : *buffers)\n> +\t\tdelete buffer;\n> +\n> +\tbuffers->clear();\n> +\n> +\treturn ret;\n> +}\n> +\n> +FrameBuffer *V4L2VideoDevice::createBuffer(struct v4l2_buffer buf)\n> +{\n> +\tconst unsigned int numPlanes = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? buf.length : 1;\n\nCan you break at 80 cols here?\n\n> +\tstd::vector planes;\n> +\tFrameBuffer *frame = nullptr;\n> +\n> +\tfor (unsigned int nplane = 0; nplane < numPlanes; nplane++) {\n> +\t\tint fd = exportDmaBuffer(buf.index, nplane);\n> +\t\tif (fd < 0)\n> +\t\t\tgoto out;\n> +\n> +\t\tFrameBuffer::Plane plane;\n> +\t\tplane.fd = fd;\n> +\t\tplane.length = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ?\n> +\t\t\tbuf.m.planes[nplane].length : buf.length;\n> +\n> +\t\tplanes.emplace_back(plane);\n> +\t}\n> +\n> +\tif (!planes.empty())\n> +\t\tframe = new FrameBuffer(planes);\n> +\telse\n> +\t\tLOG(V4L2, Error) << \"Failed to get planes\";\n> +out:\n> +\tfor (FrameBuffer::Plane &plane : planes)\n> +\t\t::close(plane.fd);\n\nCan the plane fd be closed -before- creating the FrameBuffer ?\nIf so you could\n\nAnway, I would return earlier if planes.empty()\n\n\tif (planes.empty()) {\n\t\tLOG(V4L2, Error) << \"Failed to get planes\";\n return nullptr;\n }\n\n\tFrameBufffer *frame = new FrameBuffer(planes);\n\tfor (FrameBuffer::Plane &plane : planes)\n\t\t::close(plane.fd);\n\n return frame;\n\nOr if you provide buffers to this operation\n buffers.emaplce_back(planes);\n\n return 0;\n\n> +\n> +\treturn frame;\n> +}\n> +\n> int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane)\n> {\n> \tstruct v4l2_exportbuffer expbuf = {};\n> @@ -1062,6 +1157,35 @@ int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane)\n> \treturn expbuf.fd;\n> }\n>\n> +/**\n> + * \\brief Operate using buffers imported from external source\n\nImport buffers externally allocated into the video device\n\n> + * \\param[in] count Number of buffers to prepare for\n\nto prepare for/to import\n\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2VideoDevice::externalBuffers(unsigned int count)\n> +{\n> +\t/*\n> +\t* We can only prepare the device to use external buffers when no\n> +\t* internal buffers have been allocated.\n> +\t*/\n> +\tif (cache_)\n> +\t\treturn 0;\n\nI would log an error, as you do the same in allocate\n> +\n> +\tint ret;\n\ndeclare when use\n\n> +\n> +\tmemoryType_ = V4L2_MEMORY_DMABUF;\n> +\n> +\tret = requestBuffers(count);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tcache_ = new V4L2BufferCache(count);\n> +\n> +\tLOG(V4L2, Debug) << \"provided for \" << count << \" external buffers\";\n\n\"provided for\" sounds weird to me\n\nThanks\n j\n\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Release all internally allocated buffers\n> */\n> --\n> 2.24.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5506D60BC4\n\tfor ;\n\tMon, 2 Dec 2019 12:24:00 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id C771A2000A;\n\tMon, 2 Dec 2019 11:23:59 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Mon, 2 Dec 2019 12:25:48 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191202112548.jizi5hqkv65ou2le@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-20-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"gnzr5audm4eh4fo4\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-20-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 19/30] libcamera: v4l2_videodevice:\n\tAdd new buffer interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 02 Dec 2019 11:24:00 -0000"}},{"id":3248,"web_url":"https://patchwork.libcamera.org/comment/3248/","msgid":"<20191215013940.GC4857@pendragon.ideasonboard.com>","date":"2019-12-15T01:39:40","subject":"Re: [libcamera-devel] [PATCH 19/30] libcamera: v4l2_videodevice:\n\tAdd new buffer interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Dec 02, 2019 at 12:25:48PM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 27, 2019 at 12:36:09AM +0100, Niklas Söderlund wrote:\n> > Extend V4L2VideoDevice with two new functions, one to deal with\n> > allocating buffers from the video device and one to prepare the video\n> > device to use external buffers.\n> >\n> > The two new functions intend to replace exportBuffers() and\n> > importBuffers() once the transition to the FrameBuffer interface is\n> > complete.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/include/v4l2_videodevice.h | 4 +\n> > src/libcamera/v4l2_videodevice.cpp | 126 ++++++++++++++++++++++-\n> > 2 files changed, 129 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > index 254f8797af42dd8a..f4cbcfbd26ea540c 100644\n> > --- a/src/libcamera/include/v4l2_videodevice.h\n> > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > @@ -162,6 +162,8 @@ public:\n> >\n> > \tint exportBuffers(BufferPool *pool);\n> > \tint importBuffers(BufferPool *pool);\n> > +\tint allocateBuffers(unsigned int count, std::vector *buffers);\n> > +\tint externalBuffers(unsigned int count);\n> \n> I agree with s/export/allocate, but why s/import/external ?\n> Furthermore allocate/import is a nice combination of names\n\nI agree with Jacopo here, externalBuffers() is a bad name. You could\nreuse the names exportBuffers() and importBuffers() as the arguments are\ndifferent. importBuffers() isn't the best name ever though, as the\nmethod doesn't really import buffers, so feel free to propose better\nnames.\n\n> > \tint releaseBuffers();\n> >\n> > \tint queueBuffer(Buffer *buffer);\n> > @@ -197,6 +199,7 @@ private:\n> > \tint requestBuffers(unsigned int count);\n> > \tint createPlane(BufferMemory *buffer, unsigned int index,\n> > \t\t\tunsigned int plane, unsigned int length);\n> > +\tFrameBuffer *createBuffer(struct v4l2_buffer buf);\n\nconst struct v4l2_buffer &buf\n\nIt should have become an automatic reflex by now :-)\n\n> > \tint exportDmaBuffer(unsigned int index, unsigned int plane);\n> >\n> > \tBuffer *dequeueBuffer();\n> > @@ -208,6 +211,7 @@ private:\n> > \tenum v4l2_memory memoryType_;\n> >\n> > \tBufferPool *bufferPool_;\n> > +\tV4L2BufferCache *cache_;\n> > \tstd::map queuedBuffers_;\n> >\n> > \tEventNotifier *fdEvent_;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index c82f2829601bd14c..9fe66018ec502626 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -377,7 +377,8 @@ const std::string V4L2DeviceFormat::toString() const\n> > * \\param[in] deviceNode The file-system path to the video device node\n> > */\n> > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > -\t: V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)\n> > +\t: V4L2Device(deviceNode), bufferPool_(nullptr), cache_(nullptr),\n> > +\t fdEvent_(nullptr)\n> > {\n> > \t/*\n> > \t * We default to an MMAP based CAPTURE video device, however this will\n> > @@ -1042,6 +1043,100 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool)\n> > \treturn 0;\n> > }\n> >\n> > +/**\n> > + * \\brief Operate using buffers allocated from local video device\n> \n> Allocate buffers from (in?) the video device\n> \n> > + * \\param[in] count Number of buffers to allocate\n> > + * \\param[out] buffers Vector to store local buffers\n> \n> s/local// ?\n\ns/local/allocated/\n\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector *buffers)\n\nI would wrap this to avoid long lines.\n\n> > +{\n> > +\tunsigned int i;\n> > +\tint ret;\n> \n> declare variables when you first use them\n> \n> > +\n> > +\tif (cache_) {\n> > +\t\tLOG(V4L2, Error) << \"Can't allocate buffers when cache exists\";\n> \n> The presence of cache_ is an indication that memory has been\n> allocated, not the sole reason of failure. I would\n> \n> \t\tLOG(V4L2, Error) << \"Can't allocate buffers multiple times\";\n> \n> Or something similar\n\n\"Buffers already allocated\" ?\n\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tmemoryType_ = V4L2_MEMORY_MMAP;\n> > +\n> > +\tret = requestBuffers(count);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tfor (i = 0; i < count; ++i) {\n> \n> Furthermore i is only used inside this loop\n\n\tfor (unsigned int i = 0 ...)\n\n> > +\t\tstruct v4l2_buffer buf = {};\n> > +\t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> > +\n> > +\t\tbuf.index = i;\n> > +\t\tbuf.type = bufferType_;\n> > +\t\tbuf.memory = memoryType_;\n> > +\t\tbuf.length = VIDEO_MAX_PLANES;\n\nARRAY_SIZE(planes) ?\n\n> > +\t\tbuf.m.planes = planes;\n> > +\n> > +\t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> > +\t\tif (ret < 0) {\n> \n> ret = -errno;\n\nThis is V4L2Device::ioctl(), not std::ioctl(), so the function returns\nthe error code.\n\n> > +\t\t\tLOG(V4L2, Error)\n> > +\t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> > +\t\t\t\t<< strerror(-ret);\n> > +\t\t\tgoto err_buf;\n> > +\t\t}\n> > +\n> > +\t\tFrameBuffer *buffer = createBuffer(buf);\n> > +\t\tif (!buffer) {\n> > +\t\t\tLOG(V4L2, Error) << \"Unable to create buffer\";\n> > +\t\t\tret = -EINVAL;\n> > +\t\t\tgoto err_buf;\n> > +\t\t}\n> > +\n> > +\t\tbuffers->push_back(buffer);\n> \n> We're dealing with pointers, so not a huge loss, but couldn't you\n> provide the buffers vector to createBuffers and make that operation\n\ns/createBuffers/createBuffer/\n\n> add the newly created buffer to the vector. In this way you could\n> return an error code..\n\nI think it would make the interface of createBuffer() a bit more\ncryptic, but it's an internal API.\n\nI'm tempted to start using ERR_PTR() but I don't think it would be a\ngood idea :-)\n\n> > +\t}\n> > +\n> > +\tcache_ = new V4L2BufferCache(*buffers);\n> > +\n> > +\treturn count;\n> \n> empty line ?\n> \n> > +err_buf:\n> > +\trequestBuffers(0);\n> > +\n> > +\tfor (FrameBuffer *buffer : *buffers)\n> > +\t\tdelete buffer;\n> > +\n> > +\tbuffers->clear();\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +FrameBuffer *V4L2VideoDevice::createBuffer(struct v4l2_buffer buf)\n\nHow about calling this method createFrameBuffer() ?\n\n> > +{\n> > +\tconst unsigned int numPlanes = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? buf.length : 1;\n> \n> Can you break at 80 cols here?\n> \n> > +\tstd::vector planes;\n> > +\tFrameBuffer *frame = nullptr;\n\ns/frame/frameBuffer/\n\n> > +\n> > +\tfor (unsigned int nplane = 0; nplane < numPlanes; nplane++) {\n> > +\t\tint fd = exportDmaBuffer(buf.index, nplane);\n> > +\t\tif (fd < 0)\n> > +\t\t\tgoto out;\n> > +\n> > +\t\tFrameBuffer::Plane plane;\n> > +\t\tplane.fd = fd;\n> > +\t\tplane.length = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ?\n> > +\t\t\tbuf.m.planes[nplane].length : buf.length;\n> > +\n> > +\t\tplanes.emplace_back(plane);\n\nThis won't save anything compared to push_back(). You should use\n\n\t\tplanes.emplace_back(plane.fd, length);\n\nwith a constructor for FrameBuffer::Plane, or use .push_back() to avoid\ngiving the false impression that the code is optimized.\n\n> > +\t}\n> > +\n> > +\tif (!planes.empty())\n> > +\t\tframe = new FrameBuffer(planes);\n> > +\telse\n> > +\t\tLOG(V4L2, Error) << \"Failed to get planes\";\n\nThis can only happen if the caller gives us a buf.length == 0. Let's\nmove this to the beginning of the method with\n\n\tif (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {\n\t\tLOG(V4L2, Error) << \"Invalid number of planes\";\n\t\treturn nullptr;\n\t}\n\n(but maybe we trust the kernel and this could be dropped completely ?)\n\nand here just do\n\n\tframeBuffer = new FrameBuffer(planes);\n\nMissing empty line.\n\n> > +out:\n> > +\tfor (FrameBuffer::Plane &plane : planes)\n> > +\t\t::close(plane.fd);\n> \n> Can the plane fd be closed -before- creating the FrameBuffer ?\n> If so you could\n> \n> Anway, I would return earlier if planes.empty()\n> \n> \tif (planes.empty()) {\n> \t\tLOG(V4L2, Error) << \"Failed to get planes\";\n> return nullptr;\n> }\n> \n> \tFrameBufffer *frame = new FrameBuffer(planes);\n> \tfor (FrameBuffer::Plane &plane : planes)\n> \t\t::close(plane.fd);\n> \n> return frame;\n> \n> Or if you provide buffers to this operation\n> buffers.emaplce_back(planes);\n> \n> return 0;\n\nAnd with proper use of the FileDescriptor class (with move semantics\nwhere appropriate), you should be able to drop the close() calls and\nreturn instead of goto out in the loop.\n\n> > +\n> > +\treturn frame;\n> > +}\n> > +\n> > int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane)\n> > {\n> > \tstruct v4l2_exportbuffer expbuf = {};\n> > @@ -1062,6 +1157,35 @@ int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane)\n> > \treturn expbuf.fd;\n> > }\n> >\n> > +/**\n> > + * \\brief Operate using buffers imported from external source\n> \n> Import buffers externally allocated into the video device\n\nHow about\n\n * \\brief Prepare the device to import \\a count buffers\n\n> > + * \\param[in] count Number of buffers to prepare for\n> \n> to prepare for/to import\n> \n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int V4L2VideoDevice::externalBuffers(unsigned int count)\n> > +{\n> > +\t/*\n> > +\t* We can only prepare the device to use external buffers when no\n> > +\t* internal buffers have been allocated.\n> > +\t*/\n\nWrong indentation.\n\n> > +\tif (cache_)\n> > +\t\treturn 0;\n> \n> I would log an error, as you do the same in allocate\n\nAnd return an error too.\n\n> > +\n> > +\tint ret;\n> \n> declare when use\n> \n> > +\n> > +\tmemoryType_ = V4L2_MEMORY_DMABUF;\n> > +\n> > +\tret = requestBuffers(count);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tcache_ = new V4L2BufferCache(count);\n> > +\n> > +\tLOG(V4L2, Debug) << \"provided for \" << count << \" external buffers\";\n> \n> \"provided for\" sounds weird to me\n\nMaybe\n\t\"Prepared to use \" << count << \" external buffers\";\n\nor\n\n\t\"Prepared to import \" << count << \" buffers\";\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > /**\n> > * \\brief Release all internally allocated buffers\n> > */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FDD4601E5\n\tfor ;\n\tSun, 15 Dec 2019 02:39:50 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC7902D1;\n\tSun, 15 Dec 2019 02:39:49 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576373990;\n\tbh=mfCNvf2g9qsl4NZOPrpVx0xmzhnBkvmBoR+PCkZFpT8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dAUH9i+xLmLLo6IVwr7T4n8IXksYcxvOPMyk/+AreLPSfqWF+wiMIUs6Wb767VyHc\n\tcLanl9k28CYvc5LlRRX4LDbYcTT7j6HNOoJ23uJKwHkoAlpDlD0Tj2ozLwBB37CW20\n\tgzidLeimlPbXGzurGioueLeTJWFbhbam2jHIH8rU=","Date":"Sun, 15 Dec 2019 03:39:40 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191215013940.GC4857@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-20-niklas.soderlund@ragnatech.se>\n\t<20191202112548.jizi5hqkv65ou2le@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191202112548.jizi5hqkv65ou2le@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 19/30] libcamera: v4l2_videodevice:\n\tAdd new buffer interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 15 Dec 2019 01:39:50 -0000"}}]