[{"id":2349,"web_url":"https://patchwork.libcamera.org/comment/2349/","msgid":"<20190808210311.GF6055@pendragon.ideasonboard.com>","date":"2019-08-08T21:03:11","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:\n> V4L2 M2M devices represent a V4L2Device with two queues. One output, and\n\ns/queues. One/queues: one/ (or \"queues, one\")\n\n> one capture on a single device node.\n> \n> Represent this by instantiating a V4L2VideoDevice for each queue type,\n> and preparing each device for its queue.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/libcamera/include/v4l2_videodevice.h |  26 ++-\n>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---\n>  2 files changed, 201 insertions(+), 25 deletions(-)\n> \n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index f5c8da93fcb5..24c58d787fde 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {\n>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT |\n>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT_MPLANE);\n>  \t}\n> +\tbool isM2M() const\n> +\t{\n> +\t\treturn device_caps() & (V4L2_CAP_VIDEO_M2M |\n> +\t\t\t\t\tV4L2_CAP_VIDEO_M2M_MPLANE);\n> +\t}\n>  \tbool isMeta() const\n>  \t{\n>  \t\treturn device_caps() & (V4L2_CAP_META_CAPTURE |\n> @@ -123,7 +128,7 @@ public:\n>  \n>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n>  \n> -\tint open();\n> +\tint open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);\n>  \tvoid close();\n>  \n>  \tconst char *driverName() const { return caps_.driver(); }\n> @@ -152,6 +157,9 @@ protected:\n>  \tstd::string logPrefix() const;\n>  \n>  private:\n> +\tint queryBufferType();\n> +\tint queryBufferType(enum v4l2_buf_type type);\n> +\n>  \tint getFormatMeta(V4L2DeviceFormat *format);\n>  \tint setFormatMeta(V4L2DeviceFormat *format);\n>  \n> @@ -182,6 +190,22 @@ private:\n>  \tEventNotifier *fdEvent_;\n>  };\n>  \n> +class V4L2M2MDevice\n> +{\n> +public:\n> +\tV4L2M2MDevice(const std::string &deviceNode);\n> +\t~V4L2M2MDevice();\n> +\n> +\tV4L2VideoDevice *output() { return output_; }\n> +\tV4L2VideoDevice *capture() { return capture_; }\n> +\tunsigned int status() { return status_; }\n\nThe status is an unsigned int, and stores an error value that can be\nnegative, so there's a problem.\n\nFurthermore, we have no status methods for the other V4L2-related\nclasses, so I think the open() should be split out from the constructor\nto make the API consistent.\n\n> +\n> +private:\n> +\tV4L2VideoDevice *output_;\n> +\tV4L2VideoDevice *capture_;\n> +\tunsigned int status_;\n> +};\n> +\n>  } /* namespace libcamera */\n>  \n>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 81098dd70190..4bd33af5f3de 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)\n>   * \\return True if the video device can capture or output images\n>   */\n>  \n> +/**\n> + * \\fn V4L2Capability::isM2M()\n> + * \\brief Identify if the device is an M2M device\n> + * \\return True if the device can capture and output images using the M2M API\n> + */\n> +\n>  /**\n>   * \\fn V4L2Capability::isMeta()\n>   * \\brief Identify if the video device captures or outputs image meta-data\n> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()\n>  \tclose();\n>  }\n>  \n> +int V4L2VideoDevice::queryBufferType()\n> +{\n> +\tif (caps_.isVideoCapture()) {\n> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +\t} else if (caps_.isVideoOutput()) {\n> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> +\t} else if (caps_.isMetaCapture()) {\n> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> +\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n> +\t} else if (caps_.isMetaOutput()) {\n> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> +\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n> +\t} else {\n> +\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)\n> +{\n> +\tswitch (type) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> +\t\tbreak;\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(V4L2, Error) << \"Unsupported buffer type\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nThese methods don't rely query the buffer type, they should be renamed.\nThe two overloaded version are also a bit confusing :-(\n\n> +\n>  /**\n>   * \\brief Open a V4L2 video device and query its capabilities\n> + *\n> + * \\param[in] fd The file descriptor to set (optional)\n> + * \\param[in] type The device type to operate on (optional)\n> + *\n> + * Opens a device or sets the file descriptor if provided, and then queries the\n> + * capabilities of the device and establishes the buffer types and device events\n> + * accordingly.\n> + *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int V4L2VideoDevice::open()\n> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)\n\nThere's very little code shared between the two implementations of\nopen(), I think I would make it two separate methods (both with the same\nname) and remove the default parameter values in the method declaration.\nThe code would be easier to read, and you could keep the\nqueryBufferType() code inlined.\n\n>  {\n>  \tint ret;\n>  \n> -\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\tif (fd != -1) {\n> +\t\tret = V4L2Device::setFd(fd);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t} else {\n> +\t\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n>  \n>  \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n>  \tif (ret < 0) {\n> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()\n>  \t * devices (POLLIN), and write notifications for OUTPUT video devices\n>  \t * (POLLOUT).\n>  \t */\n> -\tif (caps_.isVideoCapture()) {\n> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> -\t\tbufferType_ = caps_.isMultiplanar()\n> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> -\t} else if (caps_.isVideoOutput()) {\n> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> -\t\tbufferType_ = caps_.isMultiplanar()\n> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> -\t} else if (caps_.isMetaCapture()) {\n> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> -\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n> -\t} else if (caps_.isMetaOutput()) {\n> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> -\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n> -\t} else {\n> -\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tif (type != V4L2_BUF_TYPE_PRIVATE)\n\nThis check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE\nas a magic default value for the non-M2M case is quite a bit of a hack.\n\n> +\t\tqueryBufferType(type);\n> +\telse\n> +\t\tqueryBufferType();\n>  \n>  \tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n>  \tfdEvent_->setEnabled(false);\n> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>  \treturn new V4L2VideoDevice(mediaEntity);\n>  }\n>  \n> +/**\n> + * \\class V4L2M2MDevice\n> + * \\brief Memory2Memory device container\n> + *\n> + * The V4L2M2MDevice manages two V4L2Device instances on the same\n\ns/V4L2Device/V4L2VideoDevice/\n\n> + * deviceNode which operate together using two queues to implement the V4L2\n\ns/deviceNode/device node/\n\n> + * Memory to Memory API.\n> + *\n> + * V4L2M2MDevices are opened at the point they are created, and will return\n\ns/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the\nplural, otherwise doxygen won't generate links properly)\n\ns/will //\n\n> + * both a capture and an output device or neither in the event of failure.\n> + *\n> + * The two devices should be configured and started as normal V4L2Device\n\ns/should/shall/\n\n> + * instances. Closing either of the devices will require recreating a new\n> + * V4L2M2MDevice.\n\nWith the proposal to add an open() method, I would add a close() method\nto V4L2M2MDevice that closes both V4L2VideoDevice, and document here\nthat the managed V4L2VideoDevice instances shall not be closed manually.\n\n> + *\n> + * The two V4L2Device instances share a single device node which is opened at\n\ns/V4L2Device/V4L2VideoDevice/\n\n> + * the point the V4L2M2MDevice is created.\n> + */\n> +\n> +/**\n> + * \\fn V4L2M2MDevice::output\n\nMissing \\brief, and below too.\n\n> + * \\return The output V4L2Device instance\n> + */\n> +\n> +/**\n> + * \\fn V4L2M2MDevice::capture\n> + * \\return The capture V4L2Device instance\n> + */\n> +\n> +/**\n> + * \\fn V4L2M2MDevice::status\n> + * \\return The construction status of the V4L2M2MDevice\n\nYou should document what the status value contains. Or rather drop this\nmethod as it won't be needed with open(). Otherwise I'd recommend\nreplacing status() with an isValid() method that returns a bool, that\nwould make the API more explicit.\n\n> + */\n> +\n> +/**\n> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n> + *\n> + * The deviceNode will be opened and shared across two V4L2VideoDevice\n\n\"will\" ? It's done in this method, not later :-)\n\n> + * instances. One to control the output stream, and one to control the capture\n> + * stream.\n\nThe second sentence has no verb.\n\n> + *\n> + * After construction, the status() must be checked to validate the instance.\n> + */\n> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n> +\t: status_(0)\n> +{\n> +\tint fd[2] = { -1, -1 };\n> +\tint ret;\n> +\n> +\toutput_ = new V4L2VideoDevice(deviceNode);\n> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n> +\n> +\t/* Both V4L2Devices will have the same deviceNode. */\n\nMore than the same device node, they use the same file handle.\n\n\t/*\n\t * Both the output and capture V4L2Device instances use the same\n\t * file handle for the same device node.\n\t */\n\n> +\tfd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);\n> +\tif (fd[0] < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n> +\t\tgoto err;\n> +\t}\n> +\n> +\tfd[1] = dup(fd[0]);\n> +\tif (fd[1] < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to duplicate M2M device: \" << strerror(-ret);\n> +\n> +\t\tgoto err;\n> +\t}\n> +\n> +\tret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> +\tif (ret)\n> +\t\tgoto err;\n> +\n> +\tret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> +\tif (ret)\n> +\t\tgoto err;\n> +\n> +\treturn;\n> +\n> +err:\n> +\tdelete capture_;\n> +\tdelete output_;\n> +\n> +\tcapture_ = nullptr;\n> +\toutput_ = nullptr;\n> +\n> +\tstatus_ = ret;\n> +\n> +\tclose(fd[1]);\n> +\tclose(fd[0]);\n\nThis is racy. delete output_ above will close fd[0] if the\noutput_->open() call succeeded, and another thread could open a file\nthat reuses the same file descriptor before the close() call here.\n\n> +}\n> +\n> +V4L2M2MDevice::~V4L2M2MDevice()\n> +{\n> +\tdelete capture_;\n> +\tdelete output_;\n> +}\n> +\n>  } /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E39A61620\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  8 Aug 2019 23:03:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84FEFCC;\n\tThu,  8 Aug 2019 23:03:14 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565298194;\n\tbh=xu9O5MJ5+xUVdKD+X/gHsfXCGLOI+8AUjy4xbgeUfPc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WIsPmfLeLr7PJTd2Lf4vG9odL9e4VVnWvfQ9YnLmD64ItGNNrUXzQYvBXG0C9aTRF\n\tebJl5wP7ogTSnMcm/H/UcX5WWKwDX9x4V6JFXLOffu78cqQr39pkoQGsFj0NeyAgi1\n\t9jbEHJ7f7MQJ0q89bKBxgaOWf5B0A2POwJYxz/7U=","Date":"Fri, 9 Aug 2019 00:03:11 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190808210311.GF6055@pendragon.ideasonboard.com>","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190808151221.24254-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 08 Aug 2019 21:03:15 -0000"}},{"id":2354,"web_url":"https://patchwork.libcamera.org/comment/2354/","msgid":"<f7c58542-6154-8858-49d1-31e8c1d12542@ideasonboard.com>","date":"2019-08-09T08:13:42","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 08/08/2019 22:03, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:\n>> V4L2 M2M devices represent a V4L2Device with two queues. One output, and\n> \n> s/queues. One/queues: one/ (or \"queues, one\")\n> \n>> one capture on a single device node.\n>>\n>> Represent this by instantiating a V4L2VideoDevice for each queue type,\n>> and preparing each device for its queue.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>>  src/libcamera/include/v4l2_videodevice.h |  26 ++-\n>>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---\n>>  2 files changed, 201 insertions(+), 25 deletions(-)\n>>\n>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n>> index f5c8da93fcb5..24c58d787fde 100644\n>> --- a/src/libcamera/include/v4l2_videodevice.h\n>> +++ b/src/libcamera/include/v4l2_videodevice.h\n>> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {\n>>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT |\n>>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT_MPLANE);\n>>  \t}\n>> +\tbool isM2M() const\n>> +\t{\n>> +\t\treturn device_caps() & (V4L2_CAP_VIDEO_M2M |\n>> +\t\t\t\t\tV4L2_CAP_VIDEO_M2M_MPLANE);\n>> +\t}\n>>  \tbool isMeta() const\n>>  \t{\n>>  \t\treturn device_caps() & (V4L2_CAP_META_CAPTURE |\n>> @@ -123,7 +128,7 @@ public:\n>>  \n>>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n>>  \n>> -\tint open();\n>> +\tint open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);\n>>  \tvoid close();\n>>  \n>>  \tconst char *driverName() const { return caps_.driver(); }\n>> @@ -152,6 +157,9 @@ protected:\n>>  \tstd::string logPrefix() const;\n>>  \n>>  private:\n>> +\tint queryBufferType();\n>> +\tint queryBufferType(enum v4l2_buf_type type);\n>> +\n>>  \tint getFormatMeta(V4L2DeviceFormat *format);\n>>  \tint setFormatMeta(V4L2DeviceFormat *format);\n>>  \n>> @@ -182,6 +190,22 @@ private:\n>>  \tEventNotifier *fdEvent_;\n>>  };\n>>  \n>> +class V4L2M2MDevice\n>> +{\n>> +public:\n>> +\tV4L2M2MDevice(const std::string &deviceNode);\n>> +\t~V4L2M2MDevice();\n>> +\n>> +\tV4L2VideoDevice *output() { return output_; }\n>> +\tV4L2VideoDevice *capture() { return capture_; }\n>> +\tunsigned int status() { return status_; }\n> \n> The status is an unsigned int, and stores an error value that can be\n> negative, so there's a problem.\n\nAh yes, that should be int. Doesn't matter it's going to be dropped.\n\n> \n> Furthermore, we have no status methods for the other V4L2-related\n> classes, so I think the open() should be split out from the constructor\n> to make the API consistent.\n\nI'll split it. in that case I'll add close() as well.\n\n\n>> +\n>> +private:\n>> +\tV4L2VideoDevice *output_;\n>> +\tV4L2VideoDevice *capture_;\n>> +\tunsigned int status_;\n>> +};\n>> +\n>>  } /* namespace libcamera */\n>>  \n>>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 81098dd70190..4bd33af5f3de 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)\n>>   * \\return True if the video device can capture or output images\n>>   */\n>>  \n>> +/**\n>> + * \\fn V4L2Capability::isM2M()\n>> + * \\brief Identify if the device is an M2M device\n>> + * \\return True if the device can capture and output images using the M2M API\n>> + */\n>> +\n>>  /**\n>>   * \\fn V4L2Capability::isMeta()\n>>   * \\brief Identify if the video device captures or outputs image meta-data\n>> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()\n>>  \tclose();\n>>  }\n>>  \n>> +int V4L2VideoDevice::queryBufferType()\n>> +{\n>> +\tif (caps_.isVideoCapture()) {\n>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>> +\t\tbufferType_ = caps_.isMultiplanar()\n>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>> +\t} else if (caps_.isVideoOutput()) {\n>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>> +\t\tbufferType_ = caps_.isMultiplanar()\n>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>> +\t} else if (caps_.isMetaCapture()) {\n>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>> +\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n>> +\t} else if (caps_.isMetaOutput()) {\n>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>> +\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n>> +\t} else {\n>> +\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)\n>> +{\n>> +\tswitch (type) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>> +\t\tbufferType_ = caps_.isMultiplanar()\n>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>> +\t\tbreak;\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>> +\t\tbufferType_ = caps_.isMultiplanar()\n>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tLOG(V4L2, Error) << \"Unsupported buffer type\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n> \n> These methods don't rely query the buffer type, they should be renamed.\n> The two overloaded version are also a bit confusing :-(\n\nIt was the simplest way to satisfy your earlier request to share code.\nThis segment is the only part which is not shared across the two\npossible open() call paths...\n\n>> +\n>>  /**\n>>   * \\brief Open a V4L2 video device and query its capabilities\n>> + *\n>> + * \\param[in] fd The file descriptor to set (optional)\n>> + * \\param[in] type The device type to operate on (optional)\n>> + *\n>> + * Opens a device or sets the file descriptor if provided, and then queries the\n>> + * capabilities of the device and establishes the buffer types and device events\n>> + * accordingly.\n>> + *\n>>   * \\return 0 on success or a negative error code otherwise\n>>   */\n>> -int V4L2VideoDevice::open()\n>> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)\n> \n> There's very little code shared between the two implementations of\n> open(), I think I would make it two separate methods (both with the same\n> name) and remove the default parameter values in the method declaration.\n> The code would be easier to read, and you could keep the\n> queryBufferType() code inlined.\n\nOk - back to the RFC version then (with the rename)\n\n\n\n>>  {\n>>  \tint ret;\n>>  \n>> -\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n>> -\tif (ret < 0)\n>> -\t\treturn ret;\n>> +\tif (fd != -1) {\n>> +\t\tret = V4L2Device::setFd(fd);\n>> +\t\tif (ret < 0)\n>> +\t\t\treturn ret;\n>> +\t} else {\n>> +\t\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n>> +\t\tif (ret < 0)\n>> +\t\t\treturn ret;\n>> +\t}\n>>  \n>>  \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n>>  \tif (ret < 0) {\n>> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()\n>>  \t * devices (POLLIN), and write notifications for OUTPUT video devices\n>>  \t * (POLLOUT).\n>>  \t */\n>> -\tif (caps_.isVideoCapture()) {\n>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>> -\t\tbufferType_ = caps_.isMultiplanar()\n>> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>> -\t} else if (caps_.isVideoOutput()) {\n>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>> -\t\tbufferType_ = caps_.isMultiplanar()\n>> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>> -\t} else if (caps_.isMetaCapture()) {\n>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>> -\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n>> -\t} else if (caps_.isMetaOutput()) {\n>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>> -\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n>> -\t} else {\n>> -\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n>> -\t\treturn -EINVAL;\n>> -\t}\n>> +\tif (type != V4L2_BUF_TYPE_PRIVATE)\n> \n> This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE\n> as a magic default value for the non-M2M case is quite a bit of a hack.\n\n\nIt was required from your request to share the code paths, and use the\nv4l2_buf_type. It was the only value I could (half-legitimately) use to\nprevent getting:\n\n vimc.cpp:340:19: error: invalid conversion from ‘int’ to\n‘v4l2_buf_type’ [-fpermissive]\n  if (video_->open())\n\nIf the arguments are not overloaded, then we don't need to specify a\ndefault or switch on it.\n\n\n> \n>> +\t\tqueryBufferType(type);\n>> +\telse\n>> +\t\tqueryBufferType();\n>>  \n>>  \tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n>>  \tfdEvent_->setEnabled(false);\n>> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>>  \treturn new V4L2VideoDevice(mediaEntity);\n>>  }\n>>  \n>> +/**\n>> + * \\class V4L2M2MDevice\n>> + * \\brief Memory2Memory device container\n>> + *\n>> + * The V4L2M2MDevice manages two V4L2Device instances on the same\n> \n> s/V4L2Device/V4L2VideoDevice/\n> \n>> + * deviceNode which operate together using two queues to implement the V4L2\n> \n> s/deviceNode/device node/\n> \n>> + * Memory to Memory API.\n>> + *\n>> + * V4L2M2MDevices are opened at the point they are created, and will return\n> \n> s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the\n> plural, otherwise doxygen won't generate links properly)\n\n\nIndeed.\n\n\n> s/will //\n> \n>> + * both a capture and an output device or neither in the event of failure.\n>> + *\n>> + * The two devices should be configured and started as normal V4L2Device\n> \n> s/should/shall/\n> \n>> + * instances. Closing either of the devices will require recreating a new\n>> + * V4L2M2MDevice.\n> \n> With the proposal to add an open() method, I would add a close() method\n> to V4L2M2MDevice that closes both V4L2VideoDevice, and document here\n> that the managed V4L2VideoDevice instances shall not be closed manually.\n\nHaha - yes, I'd already come to the close() conclusion as well.\n\n\n> \n>> + *\n>> + * The two V4L2Device instances share a single device node which is opened at\n> \n> s/V4L2Device/V4L2VideoDevice/\n> \n>> + * the point the V4L2M2MDevice is created.\n>> + */\n>> +\n>> +/**\n>> + * \\fn V4L2M2MDevice::output\n> \n> Missing \\brief, and below too.\n\nWill add.\n\n> \n>> + * \\return The output V4L2Device instance\n>> + */\n>> +\n>> +/**\n>> + * \\fn V4L2M2MDevice::capture\n>> + * \\return The capture V4L2Device instance\n>> + */\n>> +\n>> +/**\n>> + * \\fn V4L2M2MDevice::status\n>> + * \\return The construction status of the V4L2M2MDevice\n> \n> You should document what the status value contains. Or rather drop this\n> method as it won't be needed with open(). Otherwise I'd recommend\n> replacing status() with an isValid() method that returns a bool, that\n> would make the API more explicit.\n\nI'll go with adding open()/close()\n\n\n> \n>> + */\n>> +\n>> +/**\n>> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n>> + *\n>> + * The deviceNode will be opened and shared across two V4L2VideoDevice\n> \n> \"will\" ? It's done in this method, not later :-)\n> \n>> + * instances. One to control the output stream, and one to control the capture\n>> + * stream.\n> \n> The second sentence has no verb.\n\nYes, there should be a semi-colon after instances instead of a full-stop.\n\n> \n>> + *\n>> + * After construction, the status() must be checked to validate the instance.\n>> + */\n>> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n>> +\t: status_(0)\n>> +{\n>> +\tint fd[2] = { -1, -1 };\n>> +\tint ret;\n>> +\n>> +\toutput_ = new V4L2VideoDevice(deviceNode);\n>> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n>> +\n>> +\t/* Both V4L2Devices will have the same deviceNode. */\n> \n> More than the same device node, they use the same file handle.\n\nWould you consider two handles (after they are dup()d) to be the same\nfile handle?\n\nAren't they separate handles representing the same device context which\ncan have their own lifetime?\n\n\n> \t/*\n> \t * Both the output and capture V4L2Device instances use the same\n> \t * file handle for the same device node.\n> \t */\n> \n>> +\tfd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);\n>> +\tif (fd[0] < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(V4L2, Error)\n>> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n>> +\t\tgoto err;\n>> +\t}\n>> +\n>> +\tfd[1] = dup(fd[0]);\n>> +\tif (fd[1] < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(V4L2, Error)\n>> +\t\t\t<< \"Failed to duplicate M2M device: \" << strerror(-ret);\n>> +\n>> +\t\tgoto err;\n>> +\t}\n>> +\n>> +\tret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);\n>> +\tif (ret)\n>> +\t\tgoto err;\n>> +\n>> +\tret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);\n>> +\tif (ret)\n>> +\t\tgoto err;\n>> +\n>> +\treturn;\n>> +\n>> +err:\n>> +\tdelete capture_;\n>> +\tdelete output_;\n>> +\n>> +\tcapture_ = nullptr;\n>> +\toutput_ = nullptr;\n>> +\n>> +\tstatus_ = ret;\n>> +\n>> +\tclose(fd[1]);\n>> +\tclose(fd[0]);\n> \n> This is racy. delete output_ above will close fd[0] if the\n> output_->open() call succeeded, and another thread could open a file\n> that reuses the same file descriptor before the close() call here.\n\nI'll move the dup() call to the overloaded open call, and always close\nthe local handle instead.\n\n>> +}\n>> +\n>> +V4L2M2MDevice::~V4L2M2MDevice()\n>> +{\n>> +\tdelete capture_;\n>> +\tdelete output_;\n>> +}\n>> +\n>>  } /* namespace libcamera */\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D1B160C03\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 Aug 2019 10:13:45 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCC04CC;\n\tFri,  9 Aug 2019 10:13:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565338425;\n\tbh=nkRiwzuFbBdTBJFUvBA2MLks1YqJRUw9ldtFnpNHh7c=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=O4VKnqTD6jf00THSPPP1XVnnmODHum0xyufXBd7WpTvhlODcryY+2nE1kRLt+gAWy\n\tupWdeV9Ezy3GpO1Zqkc6KChz1TsqJIm/0zyLNImkvMHAHuW69+cs44cVVooIKuvkqS\n\tn5hVsUmjdTbmPXJxNb00+F11nb6cFgTXHPskLuk8=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-4-kieran.bingham@ideasonboard.com>\n\t<20190808210311.GF6055@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<f7c58542-6154-8858-49d1-31e8c1d12542@ideasonboard.com>","Date":"Fri, 9 Aug 2019 09:13:42 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190808210311.GF6055@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 09 Aug 2019 08:13:45 -0000"}},{"id":2355,"web_url":"https://patchwork.libcamera.org/comment/2355/","msgid":"<20190809082524.GA4790@pendragon.ideasonboard.com>","date":"2019-08-09T08:25:24","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:\n> On 08/08/2019 22:03, Laurent Pinchart wrote:\n> > On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:\n> >> V4L2 M2M devices represent a V4L2Device with two queues. One output, and\n> > \n> > s/queues. One/queues: one/ (or \"queues, one\")\n> > \n> >> one capture on a single device node.\n> >>\n> >> Represent this by instantiating a V4L2VideoDevice for each queue type,\n> >> and preparing each device for its queue.\n> >>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> ---\n> >>  src/libcamera/include/v4l2_videodevice.h |  26 ++-\n> >>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---\n> >>  2 files changed, 201 insertions(+), 25 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> >> index f5c8da93fcb5..24c58d787fde 100644\n> >> --- a/src/libcamera/include/v4l2_videodevice.h\n> >> +++ b/src/libcamera/include/v4l2_videodevice.h\n> >> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {\n> >>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT |\n> >>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT_MPLANE);\n> >>  \t}\n> >> +\tbool isM2M() const\n> >> +\t{\n> >> +\t\treturn device_caps() & (V4L2_CAP_VIDEO_M2M |\n> >> +\t\t\t\t\tV4L2_CAP_VIDEO_M2M_MPLANE);\n> >> +\t}\n> >>  \tbool isMeta() const\n> >>  \t{\n> >>  \t\treturn device_caps() & (V4L2_CAP_META_CAPTURE |\n> >> @@ -123,7 +128,7 @@ public:\n> >>  \n> >>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n> >>  \n> >> -\tint open();\n> >> +\tint open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);\n> >>  \tvoid close();\n> >>  \n> >>  \tconst char *driverName() const { return caps_.driver(); }\n> >> @@ -152,6 +157,9 @@ protected:\n> >>  \tstd::string logPrefix() const;\n> >>  \n> >>  private:\n> >> +\tint queryBufferType();\n> >> +\tint queryBufferType(enum v4l2_buf_type type);\n> >> +\n> >>  \tint getFormatMeta(V4L2DeviceFormat *format);\n> >>  \tint setFormatMeta(V4L2DeviceFormat *format);\n> >>  \n> >> @@ -182,6 +190,22 @@ private:\n> >>  \tEventNotifier *fdEvent_;\n> >>  };\n> >>  \n> >> +class V4L2M2MDevice\n> >> +{\n> >> +public:\n> >> +\tV4L2M2MDevice(const std::string &deviceNode);\n> >> +\t~V4L2M2MDevice();\n> >> +\n> >> +\tV4L2VideoDevice *output() { return output_; }\n> >> +\tV4L2VideoDevice *capture() { return capture_; }\n> >> +\tunsigned int status() { return status_; }\n> > \n> > The status is an unsigned int, and stores an error value that can be\n> > negative, so there's a problem.\n> \n> Ah yes, that should be int. Doesn't matter it's going to be dropped.\n> \n> > Furthermore, we have no status methods for the other V4L2-related\n> > classes, so I think the open() should be split out from the constructor\n> > to make the API consistent.\n> \n> I'll split it. in that case I'll add close() as well.\n> \n> >> +\n> >> +private:\n> >> +\tV4L2VideoDevice *output_;\n> >> +\tV4L2VideoDevice *capture_;\n> >> +\tunsigned int status_;\n> >> +};\n> >> +\n> >>  } /* namespace libcamera */\n> >>  \n> >>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */\n> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >> index 81098dd70190..4bd33af5f3de 100644\n> >> --- a/src/libcamera/v4l2_videodevice.cpp\n> >> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)\n> >>   * \\return True if the video device can capture or output images\n> >>   */\n> >>  \n> >> +/**\n> >> + * \\fn V4L2Capability::isM2M()\n> >> + * \\brief Identify if the device is an M2M device\n> >> + * \\return True if the device can capture and output images using the M2M API\n> >> + */\n> >> +\n> >>  /**\n> >>   * \\fn V4L2Capability::isMeta()\n> >>   * \\brief Identify if the video device captures or outputs image meta-data\n> >> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()\n> >>  \tclose();\n> >>  }\n> >>  \n> >> +int V4L2VideoDevice::queryBufferType()\n> >> +{\n> >> +\tif (caps_.isVideoCapture()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> +\t} else if (caps_.isVideoOutput()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> +\t} else if (caps_.isMetaCapture()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> +\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n> >> +\t} else if (caps_.isMetaOutput()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> +\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n> >> +\t} else {\n> >> +\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)\n> >> +{\n> >> +\tswitch (type) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> +\t\tbreak;\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> +\t\tbreak;\n> >> +\tdefault:\n> >> +\t\tLOG(V4L2, Error) << \"Unsupported buffer type\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> > \n> > These methods don't rely query the buffer type, they should be renamed.\n> > The two overloaded version are also a bit confusing :-(\n> \n> It was the simplest way to satisfy your earlier request to share code.\n> This segment is the only part which is not shared across the two\n> possible open() call paths...\n\nYes, I'm sorry. I was hoping code could be shared in a clean way, but\nthe end result is more difficult to read, and thus not worth it in my\nopinion :-( Not your fault of course, just the code's fault :-)\n\n> >> +\n> >>  /**\n> >>   * \\brief Open a V4L2 video device and query its capabilities\n> >> + *\n> >> + * \\param[in] fd The file descriptor to set (optional)\n> >> + * \\param[in] type The device type to operate on (optional)\n> >> + *\n> >> + * Opens a device or sets the file descriptor if provided, and then queries the\n> >> + * capabilities of the device and establishes the buffer types and device events\n> >> + * accordingly.\n> >> + *\n> >>   * \\return 0 on success or a negative error code otherwise\n> >>   */\n> >> -int V4L2VideoDevice::open()\n> >> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)\n> > \n> > There's very little code shared between the two implementations of\n> > open(), I think I would make it two separate methods (both with the same\n> > name) and remove the default parameter values in the method declaration.\n> > The code would be easier to read, and you could keep the\n> > queryBufferType() code inlined.\n> \n> Ok - back to the RFC version then (with the rename)\n> \n> >>  {\n> >>  \tint ret;\n> >>  \n> >> -\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> >> -\tif (ret < 0)\n> >> -\t\treturn ret;\n> >> +\tif (fd != -1) {\n> >> +\t\tret = V4L2Device::setFd(fd);\n> >> +\t\tif (ret < 0)\n> >> +\t\t\treturn ret;\n> >> +\t} else {\n> >> +\t\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> >> +\t\tif (ret < 0)\n> >> +\t\t\treturn ret;\n> >> +\t}\n> >>  \n> >>  \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> >>  \tif (ret < 0) {\n> >> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()\n> >>  \t * devices (POLLIN), and write notifications for OUTPUT video devices\n> >>  \t * (POLLOUT).\n> >>  \t */\n> >> -\tif (caps_.isVideoCapture()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> -\t\tbufferType_ = caps_.isMultiplanar()\n> >> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> -\t} else if (caps_.isVideoOutput()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> -\t\tbufferType_ = caps_.isMultiplanar()\n> >> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> -\t} else if (caps_.isMetaCapture()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> -\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n> >> -\t} else if (caps_.isMetaOutput()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> -\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n> >> -\t} else {\n> >> -\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n> >> -\t\treturn -EINVAL;\n> >> -\t}\n> >> +\tif (type != V4L2_BUF_TYPE_PRIVATE)\n> > \n> > This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE\n> > as a magic default value for the non-M2M case is quite a bit of a hack.\n> \n> It was required from your request to share the code paths, and use the\n> v4l2_buf_type. It was the only value I could (half-legitimately) use to\n> prevent getting:\n> \n>  vimc.cpp:340:19: error: invalid conversion from ‘int’ to\n> ‘v4l2_buf_type’ [-fpermissive]\n>   if (video_->open())\n> \n> If the arguments are not overloaded, then we don't need to specify a\n> default or switch on it.\n> \n> >> +\t\tqueryBufferType(type);\n> >> +\telse\n> >> +\t\tqueryBufferType();\n> >>  \n> >>  \tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> >>  \tfdEvent_->setEnabled(false);\n> >> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> >>  \treturn new V4L2VideoDevice(mediaEntity);\n> >>  }\n> >>  \n> >> +/**\n> >> + * \\class V4L2M2MDevice\n> >> + * \\brief Memory2Memory device container\n> >> + *\n> >> + * The V4L2M2MDevice manages two V4L2Device instances on the same\n> > \n> > s/V4L2Device/V4L2VideoDevice/\n> > \n> >> + * deviceNode which operate together using two queues to implement the V4L2\n> > \n> > s/deviceNode/device node/\n> > \n> >> + * Memory to Memory API.\n> >> + *\n> >> + * V4L2M2MDevices are opened at the point they are created, and will return\n> > \n> > s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the\n> > plural, otherwise doxygen won't generate links properly)\n> \n> Indeed.\n> \n> > s/will //\n> > \n> >> + * both a capture and an output device or neither in the event of failure.\n> >> + *\n> >> + * The two devices should be configured and started as normal V4L2Device\n> > \n> > s/should/shall/\n> > \n> >> + * instances. Closing either of the devices will require recreating a new\n> >> + * V4L2M2MDevice.\n> > \n> > With the proposal to add an open() method, I would add a close() method\n> > to V4L2M2MDevice that closes both V4L2VideoDevice, and document here\n> > that the managed V4L2VideoDevice instances shall not be closed manually.\n> \n> Haha - yes, I'd already come to the close() conclusion as well.\n> \n> > \n> >> + *\n> >> + * The two V4L2Device instances share a single device node which is opened at\n> > \n> > s/V4L2Device/V4L2VideoDevice/\n> > \n> >> + * the point the V4L2M2MDevice is created.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::output\n> > \n> > Missing \\brief, and below too.\n> \n> Will add.\n> \n> >> + * \\return The output V4L2Device instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::capture\n> >> + * \\return The capture V4L2Device instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::status\n> >> + * \\return The construction status of the V4L2M2MDevice\n> > \n> > You should document what the status value contains. Or rather drop this\n> > method as it won't be needed with open(). Otherwise I'd recommend\n> > replacing status() with an isValid() method that returns a bool, that\n> > would make the API more explicit.\n> \n> I'll go with adding open()/close()\n> \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n> >> + *\n> >> + * The deviceNode will be opened and shared across two V4L2VideoDevice\n> > \n> > \"will\" ? It's done in this method, not later :-)\n> > \n> >> + * instances. One to control the output stream, and one to control the capture\n> >> + * stream.\n> > \n> > The second sentence has no verb.\n> \n> Yes, there should be a semi-colon after instances instead of a full-stop.\n> \n> >> + *\n> >> + * After construction, the status() must be checked to validate the instance.\n> >> + */\n> >> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n> >> +\t: status_(0)\n> >> +{\n> >> +\tint fd[2] = { -1, -1 };\n> >> +\tint ret;\n> >> +\n> >> +\toutput_ = new V4L2VideoDevice(deviceNode);\n> >> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n> >> +\n> >> +\t/* Both V4L2Devices will have the same deviceNode. */\n> > \n> > More than the same device node, they use the same file handle.\n> \n> Would you consider two handles (after they are dup()d) to be the same\n> file handle?\n> \n> Aren't they separate handles representing the same device context which\n> can have their own lifetime?\n\nWhat's the right word for the middle part of fd -> file -> inode ?\n\"file\" usually refers to a file on disk, and so does device node.\nAccording to man dup,\n\n\"After a successful return, the old and new file descriptors may be used\ninterchangeably.  They refer to the same open file description (see\nopen(2)) and thus share file offset and file status flags; for example,\nif the file offset is modified by using lseek(2) on one of the file\ndescriptors, the offset is also changed for the other.\"\n\n\"File description\" sounds weird.\n\n> > \t/*\n> > \t * Both the output and capture V4L2Device instances use the same\n> > \t * file handle for the same device node.\n> > \t */\n> > \n> >> +\tfd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);\n> >> +\tif (fd[0] < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2, Error)\n> >> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n> >> +\t\tgoto err;\n> >> +\t}\n> >> +\n> >> +\tfd[1] = dup(fd[0]);\n> >> +\tif (fd[1] < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2, Error)\n> >> +\t\t\t<< \"Failed to duplicate M2M device: \" << strerror(-ret);\n> >> +\n> >> +\t\tgoto err;\n> >> +\t}\n> >> +\n> >> +\tret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> >> +\tif (ret)\n> >> +\t\tgoto err;\n> >> +\n> >> +\tret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> >> +\tif (ret)\n> >> +\t\tgoto err;\n> >> +\n> >> +\treturn;\n> >> +\n> >> +err:\n> >> +\tdelete capture_;\n> >> +\tdelete output_;\n> >> +\n> >> +\tcapture_ = nullptr;\n> >> +\toutput_ = nullptr;\n> >> +\n> >> +\tstatus_ = ret;\n> >> +\n> >> +\tclose(fd[1]);\n> >> +\tclose(fd[0]);\n> > \n> > This is racy. delete output_ above will close fd[0] if the\n> > output_->open() call succeeded, and another thread could open a file\n> > that reuses the same file descriptor before the close() call here.\n> \n> I'll move the dup() call to the overloaded open call, and always close\n> the local handle instead.\n\nGood idea !\n\nThe other option would be to skip close() on the fd in V4L2Device if the\nfd was set with setFd(), and close it in V4L2M2MDevice::close(). This\ncouldn't be done before as there was no close() for V4L2M2MDevice. What\ndo you think would be best ?\n\n> >> +}\n> >> +\n> >> +V4L2M2MDevice::~V4L2M2MDevice()\n> >> +{\n> >> +\tdelete capture_;\n> >> +\tdelete output_;\n> >> +}\n> >> +\n> >>  } /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9591760C03\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 Aug 2019 10:25:27 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DCF59CC;\n\tFri,  9 Aug 2019 10:25:26 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565339127;\n\tbh=16QHxgYx6vu1Y1l7Tv0dnED+FQrdqnxMYB6UXUTUz/g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aQr124PZbLkPM6HrDd5tOxXGpBGOEGYD7O5zPwTfNIq/nBl+Z7PTAm72lZ5PSkWmd\n\tJeCIbaH1Ikn97N31zkpkI0pB2bLsYwBl0BrW+xl4k68gq1RKJFSGFcRnG1IY83UqMa\n\tTxpGwaaaO5ocbRYL1qi/hs2rHHQ4WvRGcXOvrWUU=","Date":"Fri, 9 Aug 2019 11:25:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190809082524.GA4790@pendragon.ideasonboard.com>","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-4-kieran.bingham@ideasonboard.com>\n\t<20190808210311.GF6055@pendragon.ideasonboard.com>\n\t<f7c58542-6154-8858-49d1-31e8c1d12542@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<f7c58542-6154-8858-49d1-31e8c1d12542@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 09 Aug 2019 08:25:27 -0000"}},{"id":2362,"web_url":"https://patchwork.libcamera.org/comment/2362/","msgid":"<20190809141223.acegys5kf457clkv@uno.localdomain>","date":"2019-08-09T14:12:23","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:\n> Hi Laurent,\n>\n> On 08/08/2019 22:03, Laurent Pinchart wrote:\n> > Hi Kieran,\n> >\n> > Thank you for the patch.\n\nI don't have other comments than Laurent's one, but...\n\n> >\n> > On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:\n> >> V4L2 M2M devices represent a V4L2Device with two queues. One output, and\n> >\n> > s/queues. One/queues: one/ (or \"queues, one\")\n> >\n> >> one capture on a single device node.\n> >>\n> >> Represent this by instantiating a V4L2VideoDevice for each queue type,\n> >> and preparing each device for its queue.\n> >>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> ---\n> >>  src/libcamera/include/v4l2_videodevice.h |  26 ++-\n> >>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---\n> >>  2 files changed, 201 insertions(+), 25 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> >> index f5c8da93fcb5..24c58d787fde 100644\n> >> --- a/src/libcamera/include/v4l2_videodevice.h\n> >> +++ b/src/libcamera/include/v4l2_videodevice.h\n> >> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {\n> >>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT |\n> >>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT_MPLANE);\n> >>  \t}\n> >> +\tbool isM2M() const\n> >> +\t{\n> >> +\t\treturn device_caps() & (V4L2_CAP_VIDEO_M2M |\n> >> +\t\t\t\t\tV4L2_CAP_VIDEO_M2M_MPLANE);\n> >> +\t}\n> >>  \tbool isMeta() const\n> >>  \t{\n> >>  \t\treturn device_caps() & (V4L2_CAP_META_CAPTURE |\n> >> @@ -123,7 +128,7 @@ public:\n> >>\n> >>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n> >>\n> >> -\tint open();\n> >> +\tint open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);\n> >>  \tvoid close();\n> >>\n> >>  \tconst char *driverName() const { return caps_.driver(); }\n> >> @@ -152,6 +157,9 @@ protected:\n> >>  \tstd::string logPrefix() const;\n> >>\n> >>  private:\n> >> +\tint queryBufferType();\n> >> +\tint queryBufferType(enum v4l2_buf_type type);\n> >> +\n> >>  \tint getFormatMeta(V4L2DeviceFormat *format);\n> >>  \tint setFormatMeta(V4L2DeviceFormat *format);\n> >>\n> >> @@ -182,6 +190,22 @@ private:\n> >>  \tEventNotifier *fdEvent_;\n> >>  };\n> >>\n> >> +class V4L2M2MDevice\n> >> +{\n> >> +public:\n> >> +\tV4L2M2MDevice(const std::string &deviceNode);\n> >> +\t~V4L2M2MDevice();\n> >> +\n> >> +\tV4L2VideoDevice *output() { return output_; }\n> >> +\tV4L2VideoDevice *capture() { return capture_; }\n> >> +\tunsigned int status() { return status_; }\n> >\n> > The status is an unsigned int, and stores an error value that can be\n> > negative, so there's a problem.\n>\n> Ah yes, that should be int. Doesn't matter it's going to be dropped.\n>\n> >\n> > Furthermore, we have no status methods for the other V4L2-related\n> > classes, so I think the open() should be split out from the constructor\n> > to make the API consistent.\n>\n> I'll split it. in that case I'll add close() as well.\n>\n>\n> >> +\n> >> +private:\n> >> +\tV4L2VideoDevice *output_;\n> >> +\tV4L2VideoDevice *capture_;\n> >> +\tunsigned int status_;\n> >> +};\n> >> +\n> >>  } /* namespace libcamera */\n> >>\n> >>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */\n> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >> index 81098dd70190..4bd33af5f3de 100644\n> >> --- a/src/libcamera/v4l2_videodevice.cpp\n> >> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)\n> >>   * \\return True if the video device can capture or output images\n> >>   */\n> >>\n> >> +/**\n> >> + * \\fn V4L2Capability::isM2M()\n> >> + * \\brief Identify if the device is an M2M device\n> >> + * \\return True if the device can capture and output images using the M2M API\n> >> + */\n> >> +\n> >>  /**\n> >>   * \\fn V4L2Capability::isMeta()\n> >>   * \\brief Identify if the video device captures or outputs image meta-data\n> >> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()\n> >>  \tclose();\n> >>  }\n> >>\n> >> +int V4L2VideoDevice::queryBufferType()\n> >> +{\n> >> +\tif (caps_.isVideoCapture()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> +\t} else if (caps_.isVideoOutput()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> +\t} else if (caps_.isMetaCapture()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> +\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n> >> +\t} else if (caps_.isMetaOutput()) {\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> +\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n> >> +\t} else {\n> >> +\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)\n> >> +{\n> >> +\tswitch (type) {\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> +\t\tbreak;\n> >> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> >> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> +\t\tbreak;\n> >> +\tdefault:\n> >> +\t\tLOG(V4L2, Error) << \"Unsupported buffer type\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >\n> > These methods don't rely query the buffer type, they should be renamed.\n> > The two overloaded version are also a bit confusing :-(\n>\n> It was the simplest way to satisfy your earlier request to share code.\n> This segment is the only part which is not shared across the two\n> possible open() call paths...\n>\n> >> +\n> >>  /**\n> >>   * \\brief Open a V4L2 video device and query its capabilities\n> >> + *\n> >> + * \\param[in] fd The file descriptor to set (optional)\n> >> + * \\param[in] type The device type to operate on (optional)\n> >> + *\n> >> + * Opens a device or sets the file descriptor if provided, and then queries the\n> >> + * capabilities of the device and establishes the buffer types and device events\n> >> + * accordingly.\n> >> + *\n> >>   * \\return 0 on success or a negative error code otherwise\n> >>   */\n> >> -int V4L2VideoDevice::open()\n> >> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)\n> >\n> > There's very little code shared between the two implementations of\n> > open(), I think I would make it two separate methods (both with the same\n> > name) and remove the default parameter values in the method declaration.\n> > The code would be easier to read, and you could keep the\n> > queryBufferType() code inlined.\n>\n> Ok - back to the RFC version then (with the rename)\n\nI too find quite confusing using the default arguments to\ndifferentiate between the use cases. I mean, not weird, but hard to\nfollow. I would overload the operation or either make a new one for\nthe purpose and open code queryBufferType there.\n\nThanks\n   j\n\n>\n>\n>\n> >>  {\n> >>  \tint ret;\n> >>\n> >> -\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> >> -\tif (ret < 0)\n> >> -\t\treturn ret;\n> >> +\tif (fd != -1) {\n> >> +\t\tret = V4L2Device::setFd(fd);\n> >> +\t\tif (ret < 0)\n> >> +\t\t\treturn ret;\n> >> +\t} else {\n> >> +\t\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> >> +\t\tif (ret < 0)\n> >> +\t\t\treturn ret;\n> >> +\t}\n> >>\n> >>  \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> >>  \tif (ret < 0) {\n> >> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()\n> >>  \t * devices (POLLIN), and write notifications for OUTPUT video devices\n> >>  \t * (POLLOUT).\n> >>  \t */\n> >> -\tif (caps_.isVideoCapture()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> -\t\tbufferType_ = caps_.isMultiplanar()\n> >> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> -\t} else if (caps_.isVideoOutput()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> -\t\tbufferType_ = caps_.isMultiplanar()\n> >> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> -\t} else if (caps_.isMetaCapture()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n> >> -\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n> >> -\t} else if (caps_.isMetaOutput()) {\n> >> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n> >> -\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n> >> -\t} else {\n> >> -\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n> >> -\t\treturn -EINVAL;\n> >> -\t}\n> >> +\tif (type != V4L2_BUF_TYPE_PRIVATE)\n> >\n> > This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE\n> > as a magic default value for the non-M2M case is quite a bit of a hack.\n>\n>\n> It was required from your request to share the code paths, and use the\n> v4l2_buf_type. It was the only value I could (half-legitimately) use to\n> prevent getting:\n>\n>  vimc.cpp:340:19: error: invalid conversion from ‘int’ to\n> ‘v4l2_buf_type’ [-fpermissive]\n>   if (video_->open())\n>\n> If the arguments are not overloaded, then we don't need to specify a\n> default or switch on it.\n>\n>\n> >\n> >> +\t\tqueryBufferType(type);\n> >> +\telse\n> >> +\t\tqueryBufferType();\n> >>\n> >>  \tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> >>  \tfdEvent_->setEnabled(false);\n> >> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> >>  \treturn new V4L2VideoDevice(mediaEntity);\n> >>  }\n> >>\n> >> +/**\n> >> + * \\class V4L2M2MDevice\n> >> + * \\brief Memory2Memory device container\n> >> + *\n> >> + * The V4L2M2MDevice manages two V4L2Device instances on the same\n> >\n> > s/V4L2Device/V4L2VideoDevice/\n> >\n> >> + * deviceNode which operate together using two queues to implement the V4L2\n> >\n> > s/deviceNode/device node/\n> >\n> >> + * Memory to Memory API.\n> >> + *\n> >> + * V4L2M2MDevices are opened at the point they are created, and will return\n> >\n> > s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the\n> > plural, otherwise doxygen won't generate links properly)\n>\n>\n> Indeed.\n>\n>\n> > s/will //\n> >\n> >> + * both a capture and an output device or neither in the event of failure.\n> >> + *\n> >> + * The two devices should be configured and started as normal V4L2Device\n> >\n> > s/should/shall/\n> >\n> >> + * instances. Closing either of the devices will require recreating a new\n> >> + * V4L2M2MDevice.\n> >\n> > With the proposal to add an open() method, I would add a close() method\n> > to V4L2M2MDevice that closes both V4L2VideoDevice, and document here\n> > that the managed V4L2VideoDevice instances shall not be closed manually.\n>\n> Haha - yes, I'd already come to the close() conclusion as well.\n>\n>\n> >\n> >> + *\n> >> + * The two V4L2Device instances share a single device node which is opened at\n> >\n> > s/V4L2Device/V4L2VideoDevice/\n> >\n> >> + * the point the V4L2M2MDevice is created.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::output\n> >\n> > Missing \\brief, and below too.\n>\n> Will add.\n>\n> >\n> >> + * \\return The output V4L2Device instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::capture\n> >> + * \\return The capture V4L2Device instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::status\n> >> + * \\return The construction status of the V4L2M2MDevice\n> >\n> > You should document what the status value contains. Or rather drop this\n> > method as it won't be needed with open(). Otherwise I'd recommend\n> > replacing status() with an isValid() method that returns a bool, that\n> > would make the API more explicit.\n>\n> I'll go with adding open()/close()\n>\n>\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n> >> + *\n> >> + * The deviceNode will be opened and shared across two V4L2VideoDevice\n> >\n> > \"will\" ? It's done in this method, not later :-)\n> >\n> >> + * instances. One to control the output stream, and one to control the capture\n> >> + * stream.\n> >\n> > The second sentence has no verb.\n>\n> Yes, there should be a semi-colon after instances instead of a full-stop.\n>\n> >\n> >> + *\n> >> + * After construction, the status() must be checked to validate the instance.\n> >> + */\n> >> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n> >> +\t: status_(0)\n> >> +{\n> >> +\tint fd[2] = { -1, -1 };\n> >> +\tint ret;\n> >> +\n> >> +\toutput_ = new V4L2VideoDevice(deviceNode);\n> >> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n> >> +\n> >> +\t/* Both V4L2Devices will have the same deviceNode. */\n> >\n> > More than the same device node, they use the same file handle.\n>\n> Would you consider two handles (after they are dup()d) to be the same\n> file handle?\n>\n> Aren't they separate handles representing the same device context which\n> can have their own lifetime?\n>\n>\n> > \t/*\n> > \t * Both the output and capture V4L2Device instances use the same\n> > \t * file handle for the same device node.\n> > \t */\n> >\n> >> +\tfd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);\n> >> +\tif (fd[0] < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2, Error)\n> >> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n> >> +\t\tgoto err;\n> >> +\t}\n> >> +\n> >> +\tfd[1] = dup(fd[0]);\n> >> +\tif (fd[1] < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2, Error)\n> >> +\t\t\t<< \"Failed to duplicate M2M device: \" << strerror(-ret);\n> >> +\n> >> +\t\tgoto err;\n> >> +\t}\n> >> +\n> >> +\tret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> >> +\tif (ret)\n> >> +\t\tgoto err;\n> >> +\n> >> +\tret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> >> +\tif (ret)\n> >> +\t\tgoto err;\n> >> +\n> >> +\treturn;\n> >> +\n> >> +err:\n> >> +\tdelete capture_;\n> >> +\tdelete output_;\n> >> +\n> >> +\tcapture_ = nullptr;\n> >> +\toutput_ = nullptr;\n> >> +\n> >> +\tstatus_ = ret;\n> >> +\n> >> +\tclose(fd[1]);\n> >> +\tclose(fd[0]);\n> >\n> > This is racy. delete output_ above will close fd[0] if the\n> > output_->open() call succeeded, and another thread could open a file\n> > that reuses the same file descriptor before the close() call here.\n>\n> I'll move the dup() call to the overloaded open call, and always close\n> the local handle instead.\n>\n> >> +}\n> >> +\n> >> +V4L2M2MDevice::~V4L2M2MDevice()\n> >> +{\n> >> +\tdelete capture_;\n> >> +\tdelete output_;\n> >> +}\n> >> +\n> >>  } /* namespace libcamera */\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09BD761623\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 Aug 2019 16:11:00 +0200 (CEST)","from uno.localdomain\n\t(host150-24-dynamic.51-79-r.retail.telecomitalia.it [79.51.24.150])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id AC33440014;\n\tFri,  9 Aug 2019 14:10:58 +0000 (UTC)"],"X-Originating-IP":"79.51.24.150","Date":"Fri, 9 Aug 2019 16:12:23 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tLibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190809141223.acegys5kf457clkv@uno.localdomain>","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-4-kieran.bingham@ideasonboard.com>\n\t<20190808210311.GF6055@pendragon.ideasonboard.com>\n\t<f7c58542-6154-8858-49d1-31e8c1d12542@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"66cavkgzmdxvfq56\"","Content-Disposition":"inline","In-Reply-To":"<f7c58542-6154-8858-49d1-31e8c1d12542@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 09 Aug 2019 14:11:00 -0000"}},{"id":2364,"web_url":"https://patchwork.libcamera.org/comment/2364/","msgid":"<c8ca4ab5-dc87-3baf-013f-8685831bc502@ideasonboard.com>","date":"2019-08-09T14:29:37","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 09/08/2019 09:25, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:\n>> On 08/08/2019 22:03, Laurent Pinchart wrote:\n>>> On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:\n>>>> V4L2 M2M devices represent a V4L2Device with two queues. One output, and\n>>>\n>>> s/queues. One/queues: one/ (or \"queues, one\")\n>>>\n\nDone\n\n>>>> one capture on a single device node.\n>>>>\n>>>> Represent this by instantiating a V4L2VideoDevice for each queue type,\n>>>> and preparing each device for its queue.\n>>>>\n>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>>> ---\n>>>>  src/libcamera/include/v4l2_videodevice.h |  26 ++-\n>>>>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---\n>>>>  2 files changed, 201 insertions(+), 25 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n>>>> index f5c8da93fcb5..24c58d787fde 100644\n>>>> --- a/src/libcamera/include/v4l2_videodevice.h\n>>>> +++ b/src/libcamera/include/v4l2_videodevice.h\n>>>> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {\n>>>>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT |\n>>>>  \t\t\t\t\tV4L2_CAP_VIDEO_OUTPUT_MPLANE);\n>>>>  \t}\n>>>> +\tbool isM2M() const\n>>>> +\t{\n>>>> +\t\treturn device_caps() & (V4L2_CAP_VIDEO_M2M |\n>>>> +\t\t\t\t\tV4L2_CAP_VIDEO_M2M_MPLANE);\n>>>> +\t}\n>>>>  \tbool isMeta() const\n>>>>  \t{\n>>>>  \t\treturn device_caps() & (V4L2_CAP_META_CAPTURE |\n>>>> @@ -123,7 +128,7 @@ public:\n>>>>  \n>>>>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n>>>>  \n>>>> -\tint open();\n>>>> +\tint open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);\n>>>>  \tvoid close();\n>>>>  \n>>>>  \tconst char *driverName() const { return caps_.driver(); }\n>>>> @@ -152,6 +157,9 @@ protected:\n>>>>  \tstd::string logPrefix() const;\n>>>>  \n>>>>  private:\n>>>> +\tint queryBufferType();\n>>>> +\tint queryBufferType(enum v4l2_buf_type type);\n>>>> +\n>>>>  \tint getFormatMeta(V4L2DeviceFormat *format);\n>>>>  \tint setFormatMeta(V4L2DeviceFormat *format);\n>>>>  \n>>>> @@ -182,6 +190,22 @@ private:\n>>>>  \tEventNotifier *fdEvent_;\n>>>>  };\n>>>>  \n>>>> +class V4L2M2MDevice\n>>>> +{\n>>>> +public:\n>>>> +\tV4L2M2MDevice(const std::string &deviceNode);\n>>>> +\t~V4L2M2MDevice();\n>>>> +\n>>>> +\tV4L2VideoDevice *output() { return output_; }\n>>>> +\tV4L2VideoDevice *capture() { return capture_; }\n>>>> +\tunsigned int status() { return status_; }\n>>>\n>>> The status is an unsigned int, and stores an error value that can be\n>>> negative, so there's a problem.\n>>\n>> Ah yes, that should be int. Doesn't matter it's going to be dropped.\n>>\n>>> Furthermore, we have no status methods for the other V4L2-related\n>>> classes, so I think the open() should be split out from the constructor\n>>> to make the API consistent.\n>>\n>> I'll split it. in that case I'll add close() as well.\n\nSplit done.\n\n\n\n>>\n>>>> +\n>>>> +private:\n>>>> +\tV4L2VideoDevice *output_;\n>>>> +\tV4L2VideoDevice *capture_;\n>>>> +\tunsigned int status_;\n>>>> +};\n>>>> +\n>>>>  } /* namespace libcamera */\n>>>>  \n>>>>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */\n>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>>> index 81098dd70190..4bd33af5f3de 100644\n>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>>> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)\n>>>>   * \\return True if the video device can capture or output images\n>>>>   */\n>>>>  \n>>>> +/**\n>>>> + * \\fn V4L2Capability::isM2M()\n>>>> + * \\brief Identify if the device is an M2M device\n>>>> + * \\return True if the device can capture and output images using the M2M API\n>>>> + */\n>>>> +\n>>>>  /**\n>>>>   * \\fn V4L2Capability::isMeta()\n>>>>   * \\brief Identify if the video device captures or outputs image meta-data\n>>>> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()\n>>>>  \tclose();\n>>>>  }\n>>>>  \n>>>> +int V4L2VideoDevice::queryBufferType()\n>>>> +{\n>>>> +\tif (caps_.isVideoCapture()) {\n>>>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>>>> +\t\tbufferType_ = caps_.isMultiplanar()\n>>>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>>>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>>>> +\t} else if (caps_.isVideoOutput()) {\n>>>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>>>> +\t\tbufferType_ = caps_.isMultiplanar()\n>>>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>>>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>>>> +\t} else if (caps_.isMetaCapture()) {\n>>>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>>>> +\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n>>>> +\t} else if (caps_.isMetaOutput()) {\n>>>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>>>> +\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n>>>> +\t} else {\n>>>> +\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n>>>> +\t\treturn -EINVAL;\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)\n>>>> +{\n>>>> +\tswitch (type) {\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>>>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>>>> +\t\tbufferType_ = caps_.isMultiplanar()\n>>>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>>>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>>>> +\t\tbreak;\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>>>> +\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>>>> +\t\tbufferType_ = caps_.isMultiplanar()\n>>>> +\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>>>> +\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>>>> +\t\tbreak;\n>>>> +\tdefault:\n>>>> +\t\tLOG(V4L2, Error) << \"Unsupported buffer type\";\n>>>> +\t\treturn -EINVAL;\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>\n>>> These methods don't rely query the buffer type, they should be renamed.\n>>> The two overloaded version are also a bit confusing :-(\n>>\n>> It was the simplest way to satisfy your earlier request to share code.\n>> This segment is the only part which is not shared across the two\n>> possible open() call paths...\n> \n> Yes, I'm sorry. I was hoping code could be shared in a clean way, but\n> the end result is more difficult to read, and thus not worth it in my\n> opinion :-( Not your fault of course, just the code's fault :-)\n\nOk - I've gone back to two open() calls. One for normal code paths, and\none for the M2M code path.\n\n\n\n> \n>>>> +\n>>>>  /**\n>>>>   * \\brief Open a V4L2 video device and query its capabilities\n>>>> + *\n>>>> + * \\param[in] fd The file descriptor to set (optional)\n>>>> + * \\param[in] type The device type to operate on (optional)\n>>>> + *\n>>>> + * Opens a device or sets the file descriptor if provided, and then queries the\n>>>> + * capabilities of the device and establishes the buffer types and device events\n>>>> + * accordingly.\n>>>> + *\n>>>>   * \\return 0 on success or a negative error code otherwise\n>>>>   */\n>>>> -int V4L2VideoDevice::open()\n>>>> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)\n>>>\n>>> There's very little code shared between the two implementations of\n>>> open(), I think I would make it two separate methods (both with the same\n>>> name) and remove the default parameter values in the method declaration.\n>>> The code would be easier to read, and you could keep the\n>>> queryBufferType() code inlined.\n>>\n>> Ok - back to the RFC version then (with the rename)\n>>\n>>>>  {\n>>>>  \tint ret;\n>>>>  \n>>>> -\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n>>>> -\tif (ret < 0)\n>>>> -\t\treturn ret;\n>>>> +\tif (fd != -1) {\n>>>> +\t\tret = V4L2Device::setFd(fd);\n>>>> +\t\tif (ret < 0)\n>>>> +\t\t\treturn ret;\n>>>> +\t} else {\n>>>> +\t\tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n>>>> +\t\tif (ret < 0)\n>>>> +\t\t\treturn ret;\n>>>> +\t}\n>>>>  \n>>>>  \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n>>>>  \tif (ret < 0) {\n>>>> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()\n>>>>  \t * devices (POLLIN), and write notifications for OUTPUT video devices\n>>>>  \t * (POLLOUT).\n>>>>  \t */\n>>>> -\tif (caps_.isVideoCapture()) {\n>>>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>>>> -\t\tbufferType_ = caps_.isMultiplanar()\n>>>> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>>>> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>>>> -\t} else if (caps_.isVideoOutput()) {\n>>>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>>>> -\t\tbufferType_ = caps_.isMultiplanar()\n>>>> -\t\t\t    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>>>> -\t\t\t    : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>>>> -\t} else if (caps_.isMetaCapture()) {\n>>>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Read);\n>>>> -\t\tbufferType_ = V4L2_BUF_TYPE_META_CAPTURE;\n>>>> -\t} else if (caps_.isMetaOutput()) {\n>>>> -\t\tfdEvent_ = new EventNotifier(fd(), EventNotifier::Write);\n>>>> -\t\tbufferType_ = V4L2_BUF_TYPE_META_OUTPUT;\n>>>> -\t} else {\n>>>> -\t\tLOG(V4L2, Error) << \"Device is not a supported type\";\n>>>> -\t\treturn -EINVAL;\n>>>> -\t}\n>>>> +\tif (type != V4L2_BUF_TYPE_PRIVATE)\n>>>\n>>> This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE\n>>> as a magic default value for the non-M2M case is quite a bit of a hack.\n>>\n>> It was required from your request to share the code paths, and use the\n>> v4l2_buf_type. It was the only value I could (half-legitimately) use to\n>> prevent getting:\n>>\n>>  vimc.cpp:340:19: error: invalid conversion from ‘int’ to\n>> ‘v4l2_buf_type’ [-fpermissive]\n>>   if (video_->open())\n>>\n>> If the arguments are not overloaded, then we don't need to specify a\n>> default or switch on it.\n>>\n>>>> +\t\tqueryBufferType(type);\n>>>> +\telse\n>>>> +\t\tqueryBufferType();\n>>>>  \n>>>>  \tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n>>>>  \tfdEvent_->setEnabled(false);\n>>>> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>>>>  \treturn new V4L2VideoDevice(mediaEntity);\n>>>>  }\n>>>>  \n>>>> +/**\n>>>> + * \\class V4L2M2MDevice\n>>>> + * \\brief Memory2Memory device container\n>>>> + *\n>>>> + * The V4L2M2MDevice manages two V4L2Device instances on the same\n>>>\n>>> s/V4L2Device/V4L2VideoDevice/\n>>>\n>>>> + * deviceNode which operate together using two queues to implement the V4L2\n>>>\n>>> s/deviceNode/device node/\n>>>\n>>>> + * Memory to Memory API.\n>>>> + *\n>>>> + * V4L2M2MDevices are opened at the point they are created, and will return\n>>>\n>>> s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the\n>>> plural, otherwise doxygen won't generate links properly)\n>>\n>> Indeed.\n>>\n>>> s/will //\n>>>\n>>>> + * both a capture and an output device or neither in the event of failure.\n>>>> + *\n>>>> + * The two devices should be configured and started as normal V4L2Device\n>>>\n>>> s/should/shall/\n>>>\n>>>> + * instances. Closing either of the devices will require recreating a new\n>>>> + * V4L2M2MDevice.\n>>>\n>>> With the proposal to add an open() method, I would add a close() method\n>>> to V4L2M2MDevice that closes both V4L2VideoDevice, and document here\n>>> that the managed V4L2VideoDevice instances shall not be closed manually.\n>>\n>> Haha - yes, I'd already come to the close() conclusion as well.\n>>\n>>>\n>>>> + *\n>>>> + * The two V4L2Device instances share a single device node which is opened at\n>>>\n>>> s/V4L2Device/V4L2VideoDevice/\n>>>\n>>>> + * the point the V4L2M2MDevice is created.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2M2MDevice::output\n>>>\n>>> Missing \\brief, and below too.\n>>\n>> Will add.\n>>\n>>>> + * \\return The output V4L2Device instance\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2M2MDevice::capture\n>>>> + * \\return The capture V4L2Device instance\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2M2MDevice::status\n>>>> + * \\return The construction status of the V4L2M2MDevice\n>>>\n>>> You should document what the status value contains. Or rather drop this\n>>> method as it won't be needed with open(). Otherwise I'd recommend\n>>> replacing status() with an isValid() method that returns a bool, that\n>>> would make the API more explicit.\n>>\n>> I'll go with adding open()/close()\n>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n>>>> + *\n>>>> + * The deviceNode will be opened and shared across two V4L2VideoDevice\n>>>\n>>> \"will\" ? It's done in this method, not later :-)\n>>>\n>>>> + * instances. One to control the output stream, and one to control the capture\n>>>> + * stream.\n>>>\n>>> The second sentence has no verb.\n>>\n>> Yes, there should be a semi-colon after instances instead of a full-stop.\n>>\n>>>> + *\n>>>> + * After construction, the status() must be checked to validate the instance.\n>>>> + */\n>>>> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n>>>> +\t: status_(0)\n>>>> +{\n>>>> +\tint fd[2] = { -1, -1 };\n>>>> +\tint ret;\n>>>> +\n>>>> +\toutput_ = new V4L2VideoDevice(deviceNode);\n>>>> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n>>>> +\n>>>> +\t/* Both V4L2Devices will have the same deviceNode. */\n>>>\n>>> More than the same device node, they use the same file handle.\n>>\n>> Would you consider two handles (after they are dup()d) to be the same\n>> file handle?\n>>\n>> Aren't they separate handles representing the same device context which\n>> can have their own lifetime?\n> \n> What's the right word for the middle part of fd -> file -> inode ?\n> \"file\" usually refers to a file on disk, and so does device node.\n> According to man dup,\n> \n> \"After a successful return, the old and new file descriptors may be used\n> interchangeably.  They refer to the same open file description (see\n> open(2)) and thus share file offset and file status flags; for example,\n> if the file offset is modified by using lseek(2) on one of the file\n> descriptors, the offset is also changed for the other.\"\n> \n> \"File description\" sounds weird.\n\nI've added\n\n/*\n * The output and capture V4L2VideoDevice instances use the same file\n * handle for the same device node. The local file handle can be closed\n * as the V4L2VideoDevice::open() retains a handle by duplicating the\n * fd passed in.\n */\n\n(which is correct in v2, where the dup happens in the open(fd, type) call)\n\n\n> \n>>> \t/*\n>>> \t * Both the output and capture V4L2Device instances use the same\n>>> \t * file handle for the same device node.\n>>> \t */\n>>>\n>>>> +\tfd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);\n>>>> +\tif (fd[0] < 0) {\n>>>> +\t\tret = -errno;\n>>>> +\t\tLOG(V4L2, Error)\n>>>> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n>>>> +\t\tgoto err;\n>>>> +\t}\n>>>> +\n>>>> +\tfd[1] = dup(fd[0]);\n>>>> +\tif (fd[1] < 0) {\n>>>> +\t\tret = -errno;\n>>>> +\t\tLOG(V4L2, Error)\n>>>> +\t\t\t<< \"Failed to duplicate M2M device: \" << strerror(-ret);\n>>>> +\n>>>> +\t\tgoto err;\n>>>> +\t}\n>>>> +\n>>>> +\tret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);\n>>>> +\tif (ret)\n>>>> +\t\tgoto err;\n>>>> +\n>>>> +\tret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);\n>>>> +\tif (ret)\n>>>> +\t\tgoto err;\n>>>> +\n>>>> +\treturn;\n>>>> +\n>>>> +err:\n>>>> +\tdelete capture_;\n>>>> +\tdelete output_;\n>>>> +\n>>>> +\tcapture_ = nullptr;\n>>>> +\toutput_ = nullptr;\n>>>> +\n>>>> +\tstatus_ = ret;\n>>>> +\n>>>> +\tclose(fd[1]);\n>>>> +\tclose(fd[0]);\n>>>\n>>> This is racy. delete output_ above will close fd[0] if the\n>>> output_->open() call succeeded, and another thread could open a file\n>>> that reuses the same file descriptor before the close() call here.\n>>\n>> I'll move the dup() call to the overloaded open call, and always close\n>> the local handle instead.\n> \n> Good idea !\n> \n> The other option would be to skip close() on the fd in V4L2Device if the\n> fd was set with setFd(), and close it in V4L2M2MDevice::close(). This\n> couldn't be done before as there was no close() for V4L2M2MDevice. What\n> do you think would be best ?\n\nI prefer handling the dup in the open call so that each device clearly\n'takes' it's own handle.\n\nAnd that's what I've done.\n\nV2 post imminent.\n\n\n\n>>>> +}\n>>>> +\n>>>> +V4L2M2MDevice::~V4L2M2MDevice()\n>>>> +{\n>>>> +\tdelete capture_;\n>>>> +\tdelete output_;\n>>>> +}\n>>>> +\n>>>>  } /* namespace libcamera */\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C33D061623\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 Aug 2019 16:29:40 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3CE99CC;\n\tFri,  9 Aug 2019 16:29:40 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565360980;\n\tbh=UwHZZoW0+oCADv2jbSfGjh4GpGYDA3QK5fmT9+C+Pr8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=sN3Yz0xRlCpjzFVK3WYpclMg0OHaMbK2TMARUQFHeTHBZUS9XMhVk0iGUua/D9GVm\n\tz9GIZSnMwNSSgHc3jEX/0ybgP/KqI006Nf+sdjZCgUd9eDcZqu701Mg7mtz4En/zk3\n\tf6wPL+lsNlyjq0eEDDz7ojEvCKbHSm/+0M/arU3A=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-4-kieran.bingham@ideasonboard.com>\n\t<20190808210311.GF6055@pendragon.ideasonboard.com>\n\t<f7c58542-6154-8858-49d1-31e8c1d12542@ideasonboard.com>\n\t<20190809082524.GA4790@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<c8ca4ab5-dc87-3baf-013f-8685831bc502@ideasonboard.com>","Date":"Fri, 9 Aug 2019 15:29:37 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190809082524.GA4790@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice:\n\tSupport M2M devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 09 Aug 2019 14:29:41 -0000"}}]