[{"id":2372,"web_url":"https://patchwork.libcamera.org/comment/2372/","msgid":"<20190810134110.GA4788@pendragon.ideasonboard.com>","date":"2019-08-10T13:41:10","subject":"Re: [libcamera-devel] [PATCH v2 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 Fri, Aug 09, 2019 at 04:04:56PM +0100, Kieran Bingham wrote:\n> V4L2 M2M devices represent a V4L2Device with two queues: One output, and\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 |  28 ++++\n>  src/libcamera/v4l2_videodevice.cpp       | 186 +++++++++++++++++++++++\n>  2 files changed, 214 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index f5c8da93fcb5..72dc8c63e4bb 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> @@ -124,6 +129,7 @@ public:\n>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n>  \n>  \tint open();\n> +\tint open(int handle, enum v4l2_buf_type type);\n>  \tvoid close();\n>  \n>  \tconst char *driverName() const { return caps_.driver(); }\n> @@ -152,6 +158,9 @@ protected:\n>  \tstd::string logPrefix() const;\n>  \n>  private:\n> +\tint queryBufferType();\n> +\tint queryBufferType(enum v4l2_buf_type type);\n\nI think these are leftovers.\n\n> +\n>  \tint getFormatMeta(V4L2DeviceFormat *format);\n>  \tint setFormatMeta(V4L2DeviceFormat *format);\n>  \n> @@ -182,6 +191,25 @@ private:\n>  \tEventNotifier *fdEvent_;\n>  };\n>  \n> +class V4L2M2MDevice\n> +{\n> +public:\n> +\tV4L2M2MDevice(const std::string &deviceNode);\n> +\t~V4L2M2MDevice();\n> +\n> +\tint open();\n> +\tvoid close();\n> +\n> +\tV4L2VideoDevice *output() { return output_; }\n> +\tV4L2VideoDevice *capture() { return capture_; }\n> +\n> +private:\n> +\tstd::string deviceNode_;\n> +\n> +\tV4L2VideoDevice *output_;\n> +\tV4L2VideoDevice *capture_;\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..9c5638995577 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> @@ -296,6 +302,10 @@ V4L2VideoDevice::~V4L2VideoDevice()\n>  \n>  /**\n>   * \\brief Open a V4L2 video device and query its capabilities\n> + *\n> + * Opens a video device, then queries the capabilities of the device and\n\ns/Opens/This method opens/\n\n> + * establishes the buffer types and device events accordingly.\n\nThe last part is internal matters I think. I would just drop this\nparagraph, and update the brief to state \"Open a V4L2 video device node\nand query its capabilities\".\n\n> + *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n>  int V4L2VideoDevice::open()\n> @@ -355,6 +365,83 @@ int V4L2VideoDevice::open()\n>  \treturn 0;\n>  }\n>  \n> +/**\n> + * \\brief Open a V4L2 video device and query its capabilities\n\nAnd here \"Open a V4L2 video device from an opened file handle and query\nits capabilities\".\n\n> + *\n\nNo need for this blank line.\n\n> + * \\param[in] handle The file descriptor to set\n\ns/handle/fd/ ?\n\n> + * \\param[in] type The device type to operate on\n> + *\n> + * Sets the file descriptor for the device and then queries the capabilities of\n\ns/Sets/This method sets/\n\nYou need a subject before a conjugated verb.\n\n> + * the device and establishes the buffer types and device events accordingly.\n\n * This methods opens a video device from the existing file descriptor \\a fd.\n * Like open() queries the capabilities of the device, but handles it according\n * to the given device \\a type instead of determining its type from the\n * capabilities. This can be used to force a given device type for M2M devices.\n *\n * The file descriptor \\a fd is duplicated, and the caller shall close \\a fd\n * when it has no further use for it. The close() method will close the\n * duplicated file descriptor, leaving \\a fd untouched.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> +{\n> +\tint ret;\n> +\tint newFd;\n> +\n> +\tnewFd = dup(handle);\n> +\tif (newFd < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> +\t\t\t\t << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = V4L2Device::setFd(newFd);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n> +\t\t\t\t << strerror(-ret);\n\nYou need to ::close(newFd) here.\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to query device capabilities: \"\n> +\t\t\t<< strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (!caps_.hasStreaming()) {\n> +\t\tLOG(V4L2, Error) << \"Device does not support streaming I/O\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/*\n> +\t * Set buffer type and wait for read notifications on CAPTURE video\n> +\t * devices (POLLIN), and write notifications for OUTPUT video devices\n> +\t * (POLLOUT).\n> +\t */\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> +\tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> +\tfdEvent_->setEnabled(false);\n> +\n> +\tLOG(V4L2, Debug)\n> +\t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> +\t\t<< caps_.driver() << \": \" << caps_.card();\n> +\n> +\treturn 0;\n> +}\n> +\n>  /**\n>   * \\brief Close the video device, releasing any resources acquired by open()\n>   */\n> @@ -1143,4 +1230,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>  \treturn new V4L2VideoDevice(mediaEntity);\n>  }\n>  \n> +/**\n> + * \\class V4L2M2MDevice\n> + * \\brief Memory2Memory video device\n> + *\n> + * The V4L2M2MDevice manages two V4L2VideoDevice instances on the same\n> + * deviceNode which operate together using two queues to implement the V4L2\n> + * Memory to Memory API.\n> + *\n> + * The two devices should be opened by calling open() on the V4L2M2MDevice, and\n> + * can be closed by calling close on the V4L2M2MDevice.\n> + *\n> + * Calling V4L2VideoDevice::open() and V4L2VideoDevice::close() on the capture\n> + * or output V4L2VideoDevice is not permitted.\n> + */\n> +\n> +/**\n> + * \\fn V4L2M2MDevice::output\n> + * \\brief Provide access to the output V4L2VideoDevice instance\n\ns/Provide access to/Retrieve/\n\n> + * \\return The output V4L2Device instance\n> + */\n> +\n> +/**\n> + * \\fn V4L2M2MDevice::capture\n> + * \\brief Provide access to the capture V4L2VideoDevice instance\n\ns/Provide access to/Retrieve/\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> + * \\return The capture V4L2Device instance\n> + */\n> +\n> +/**\n> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n> + */\n> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n> +\t: deviceNode_(deviceNode)\n> +{\n> +\toutput_ = new V4L2VideoDevice(deviceNode);\n> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n> +}\n> +\n> +V4L2M2MDevice::~V4L2M2MDevice()\n> +{\n> +\tdelete capture_;\n> +\tdelete output_;\n> +}\n> +\n> +/**\n> + * \\brief Open a V4L2 Memory to Memory device\n> + *\n> + * Open the device node and prepare the two V4L2VideoDevice instances to handle\n> + * their respective buffer queues.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2M2MDevice::open()\n> +{\n> +\tint fd;\n> +\tint ret;\n> +\n> +\t/*\n> +\t * The output and capture V4L2VideoDevice instances use the same file\n> +\t * handle for the same device node. The local file handle can be closed\n> +\t * as the V4L2VideoDevice::open() retains a handle by duplicating the\n> +\t * fd passed in.\n> +\t */\n> +\tfd = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);\n> +\tif (fd < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> +\tif (ret)\n> +\t\tgoto err;\n> +\n> +\tret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> +\tif (ret)\n> +\t\tgoto err;\n> +\n> +\t::close(fd);\n> +\n> +\treturn 0;\n> +\n> +err:\n> +\tclose();\n> +\t::close(fd);\n> +\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\brief Close the memory-to-memory device, releasing any resources acquired by\n> + * open()\n> + */\n> +void V4L2M2MDevice::close()\n> +{\n> +\tcapture_->close();\n> +\toutput_->close();\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 8D79A60C03\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 10 Aug 2019 15:41:13 +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 EF48167;\n\tSat, 10 Aug 2019 15:41:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565444473;\n\tbh=+E64/hVfQq63vKEv6x2/jsm3RcUlJ6l4rCmmaDt0CZg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=riVfg4zoB6pYVTFT+uwwioU5CoXv4mQbMeRzjd16DJRZAPQmJmFItqmeB9leXn5MI\n\tsVgxeRgjgVQsER7jW8UTG8czhB6zxCDs+brBGP2qjSWlR+62/0Uge85bdmuz95h51s\n\t07/gnh7xc/NV+EEb0RxTY3nROGB0YD+nOVU2hoi0=","Date":"Sat, 10 Aug 2019 16:41:10 +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":"<20190810134110.GA4788@pendragon.ideasonboard.com>","References":"<20190809150459.14421-1-kieran.bingham@ideasonboard.com>\n\t<20190809150459.14421-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190809150459.14421-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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":"Sat, 10 Aug 2019 13:41:13 -0000"}},{"id":2380,"web_url":"https://patchwork.libcamera.org/comment/2380/","msgid":"<3c82e353-5d68-cc3d-461d-aa8b4d05eedf@ideasonboard.com>","date":"2019-08-12T09:01:53","subject":"Re: [libcamera-devel] [PATCH v2 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 10/08/2019 14:41, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 09, 2019 at 04:04:56PM +0100, Kieran Bingham wrote:\n>> V4L2 M2M devices represent a V4L2Device with two queues: One output, and\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 |  28 ++++\n>>  src/libcamera/v4l2_videodevice.cpp       | 186 +++++++++++++++++++++++\n>>  2 files changed, 214 insertions(+)\n>>\n>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n>> index f5c8da93fcb5..72dc8c63e4bb 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>> @@ -124,6 +129,7 @@ public:\n>>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n>>  \n>>  \tint open();\n>> +\tint open(int handle, enum v4l2_buf_type type);\n>>  \tvoid close();\n>>  \n>>  \tconst char *driverName() const { return caps_.driver(); }\n>> @@ -152,6 +158,9 @@ protected:\n>>  \tstd::string logPrefix() const;\n>>  \n>>  private:\n>> +\tint queryBufferType();\n>> +\tint queryBufferType(enum v4l2_buf_type type);\n> \n> I think these are leftovers.\n\nYes, removed.\n\n\n>> +\n>>  \tint getFormatMeta(V4L2DeviceFormat *format);\n>>  \tint setFormatMeta(V4L2DeviceFormat *format);\n>>  \n>> @@ -182,6 +191,25 @@ private:\n>>  \tEventNotifier *fdEvent_;\n>>  };\n>>  \n>> +class V4L2M2MDevice\n>> +{\n>> +public:\n>> +\tV4L2M2MDevice(const std::string &deviceNode);\n>> +\t~V4L2M2MDevice();\n>> +\n>> +\tint open();\n>> +\tvoid close();\n>> +\n>> +\tV4L2VideoDevice *output() { return output_; }\n>> +\tV4L2VideoDevice *capture() { return capture_; }\n>> +\n>> +private:\n>> +\tstd::string deviceNode_;\n>> +\n>> +\tV4L2VideoDevice *output_;\n>> +\tV4L2VideoDevice *capture_;\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..9c5638995577 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>> @@ -296,6 +302,10 @@ V4L2VideoDevice::~V4L2VideoDevice()\n>>  \n>>  /**\n>>   * \\brief Open a V4L2 video device and query its capabilities\n>> + *\n>> + * Opens a video device, then queries the capabilities of the device and\n> \n> s/Opens/This method opens/\n> \n>> + * establishes the buffer types and device events accordingly.\n> \n> The last part is internal matters I think. I would just drop this\n> paragraph, and update the brief to state \"Open a V4L2 video device node\n> and query its capabilities\".\n\n\nYou mean simply add the word 'node' to the existing brief?\n\nThe 'a' doesn't give enough context in that case any more (or perhaps\nbefore), because it only opens 'the' device node that exists in this\nclass instance. It doesn't open any device node. Only the one which was\npreviously set in the constructor.\n\n\nI'll change it to :\n\n  \"open /the/ V4L2 video device node and ...\"\n\n\n>> + *\n>>   * \\return 0 on success or a negative error code otherwise\n>>   */\n>>  int V4L2VideoDevice::open()\n>> @@ -355,6 +365,83 @@ int V4L2VideoDevice::open()\n>>  \treturn 0;\n>>  }\n>>  \n>> +/**\n>> + * \\brief Open a V4L2 video device and query its capabilities\n> \n> And here \"Open a V4L2 video device from an opened file handle and query\n> its capabilities\".\n\nChanged.\n\n> \n>> + *\n> \n> No need for this blank line.\n> \n>> + * \\param[in] handle The file descriptor to set\n> \n> s/handle/fd/ ?\n\nNo,.\n\n\n>> + * \\param[in] type The device type to operate on\n>> + *\n>> + * Sets the file descriptor for the device and then queries the capabilities of\n> \n> s/Sets/This method sets/\n> \n> You need a subject before a conjugated verb.\n\nWe're /in/ the subject. It can be implicit.\n\n\n>> + * the device and establishes the buffer types and device events accordingly.\n> \n>  * This methods opens a video device from the existing file descriptor \\a fd.\n\nWe can't use fd as an argument, because we use fd().\nWe can't use newFd, because that's the result of the dup().\n\nThat's why I've used handle.\n\n\n>  * Like open() queries the capabilities of the device, but handles it according\n\n                ^\nIsn't this the same issue?\n\n\n>  * to the given device \\a type instead of determining its type from the\n>  * capabilities. This can be used to force a given device type for M2M devices.\n>  *\n>  * The file descriptor \\a fd is duplicated, and the caller shall close \\a fd\n>  * when it has no further use for it. The close() method will close the\n>  * duplicated file descriptor, leaving \\a fd untouched.\n\nChanged to:\n\n * This methods opens a video device from the existing file descriptor \\a\n * handle. Like open(), this method queries the capabilities of the\ndevice, but\n * handles it according to the given device \\a type instead of\ndetermining its\n * type from the capabilities. This can be used to force a given device\ntype for\n * M2M devices.\n *\n * The file descriptor \\a handle is duplicated, and the caller is\nresponsible\n * for closing the \\a handle when it has no further use for it. The close()\n * method will close the duplicated file descriptor, leaving \\a handle\n * untouched.\n\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n>> +{\n>> +\tint ret;\n>> +\tint newFd;\n>> +\n>> +\tnewFd = dup(handle);\n>> +\tif (newFd < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n>> +\t\t\t\t << strerror(-ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tret = V4L2Device::setFd(newFd);\n>> +\tif (ret < 0) {\n>> +\t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n>> +\t\t\t\t << strerror(-ret);\n> \n> You need to ::close(newFd) here.\n> \n\nAh yes.\n\n\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n>> +\tif (ret < 0) {\n>> +\t\tLOG(V4L2, Error)\n>> +\t\t\t<< \"Failed to query device capabilities: \"\n>> +\t\t\t<< strerror(-ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tif (!caps_.hasStreaming()) {\n>> +\t\tLOG(V4L2, Error) << \"Device does not support streaming I/O\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Set buffer type and wait for read notifications on CAPTURE video\n>> +\t * devices (POLLIN), and write notifications for OUTPUT video devices\n>> +\t * (POLLOUT).\n>> +\t */\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>> +\tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n>> +\tfdEvent_->setEnabled(false);\n>> +\n>> +\tLOG(V4L2, Debug)\n>> +\t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n>> +\t\t<< caps_.driver() << \": \" << caps_.card();\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>>  /**\n>>   * \\brief Close the video device, releasing any resources acquired by open()\n>>   */\n>> @@ -1143,4 +1230,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>>  \treturn new V4L2VideoDevice(mediaEntity);\n>>  }\n>>  \n>> +/**\n>> + * \\class V4L2M2MDevice\n>> + * \\brief Memory2Memory video device\n>> + *\n>> + * The V4L2M2MDevice manages two V4L2VideoDevice instances on the same\n>> + * deviceNode which operate together using two queues to implement the V4L2\n>> + * Memory to Memory API.\n>> + *\n>> + * The two devices should be opened by calling open() on the V4L2M2MDevice, and\n>> + * can be closed by calling close on the V4L2M2MDevice.\n>> + *\n>> + * Calling V4L2VideoDevice::open() and V4L2VideoDevice::close() on the capture\n>> + * or output V4L2VideoDevice is not permitted.\n>> + */\n>> +\n>> +/**\n>> + * \\fn V4L2M2MDevice::output\n>> + * \\brief Provide access to the output V4L2VideoDevice instance\n> \n> s/Provide access to/Retrieve/\n\n\nChanged.\n\n>> + * \\return The output V4L2Device instance\n>> + */\n>> +\n>> +/**\n>> + * \\fn V4L2M2MDevice::capture\n>> + * \\brief Provide access to the capture V4L2VideoDevice instance\n> \n> s/Provide access to/Retrieve/\n\nChanged.\n\n\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nThanks,\n\n\n>> + * \\return The capture V4L2Device instance\n>> + */\n>> +\n>> +/**\n>> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n>> + */\n>> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n>> +\t: deviceNode_(deviceNode)\n>> +{\n>> +\toutput_ = new V4L2VideoDevice(deviceNode);\n>> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n>> +}\n>> +\n>> +V4L2M2MDevice::~V4L2M2MDevice()\n>> +{\n>> +\tdelete capture_;\n>> +\tdelete output_;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Open a V4L2 Memory to Memory device\n>> + *\n>> + * Open the device node and prepare the two V4L2VideoDevice instances to handle\n>> + * their respective buffer queues.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int V4L2M2MDevice::open()\n>> +{\n>> +\tint fd;\n>> +\tint ret;\n>> +\n>> +\t/*\n>> +\t * The output and capture V4L2VideoDevice instances use the same file\n>> +\t * handle for the same device node. The local file handle can be closed\n>> +\t * as the V4L2VideoDevice::open() retains a handle by duplicating the\n>> +\t * fd passed in.\n>> +\t */\n>> +\tfd = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);\n>> +\tif (fd < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(V4L2, Error)\n>> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);\n>> +\tif (ret)\n>> +\t\tgoto err;\n>> +\n>> +\tret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);\n>> +\tif (ret)\n>> +\t\tgoto err;\n>> +\n>> +\t::close(fd);\n>> +\n>> +\treturn 0;\n>> +\n>> +err:\n>> +\tclose();\n>> +\t::close(fd);\n>> +\n>> +\treturn ret;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Close the memory-to-memory device, releasing any resources acquired by\n>> + * open()\n>> + */\n>> +void V4L2M2MDevice::close()\n>> +{\n>> +\tcapture_->close();\n>> +\toutput_->close();\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 DBD4460E38\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 12 Aug 2019 11:01:56 +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 25A90327;\n\tMon, 12 Aug 2019 11:01:56 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565600516;\n\tbh=7OUy1xb5zHRdvXmkuxOnfAU5FMlnxqNENQIQPPVxDTI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=IPJr+qKWzyL4YZ9s0XVFPD18se4gzzUvBIAnKP+jBVQVYgcAYDkguVWVwIGFRpCXy\n\t4rUsV9d/MoobAxFrf9s7B+nl8yoqT5XsQw3JNSHPEiuSnKpRQnEWfXtvTvxibVxzON\n\tr07blTiZr3mw8WDcsnMx2oGw5REY6LNZwWpiz0Vk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190809150459.14421-1-kieran.bingham@ideasonboard.com>\n\t<20190809150459.14421-4-kieran.bingham@ideasonboard.com>\n\t<20190810134110.GA4788@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":"<3c82e353-5d68-cc3d-461d-aa8b4d05eedf@ideasonboard.com>","Date":"Mon, 12 Aug 2019 10:01:53 +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":"<20190810134110.GA4788@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 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":"Mon, 12 Aug 2019 09:01:57 -0000"}},{"id":2390,"web_url":"https://patchwork.libcamera.org/comment/2390/","msgid":"<20190812111637.GE5006@pendragon.ideasonboard.com>","date":"2019-08-12T11:16:37","subject":"Re: [libcamera-devel] [PATCH v2 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 Mon, Aug 12, 2019 at 10:01:53AM +0100, Kieran Bingham wrote:\n> On 10/08/2019 14:41, Laurent Pinchart wrote:\n> > On Fri, Aug 09, 2019 at 04:04:56PM +0100, Kieran Bingham wrote:\n> >> V4L2 M2M devices represent a V4L2Device with two queues: One output, and\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 |  28 ++++\n> >>  src/libcamera/v4l2_videodevice.cpp       | 186 +++++++++++++++++++++++\n> >>  2 files changed, 214 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> >> index f5c8da93fcb5..72dc8c63e4bb 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> >> @@ -124,6 +129,7 @@ public:\n> >>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n> >>  \n> >>  \tint open();\n> >> +\tint open(int handle, enum v4l2_buf_type type);\n> >>  \tvoid close();\n> >>  \n> >>  \tconst char *driverName() const { return caps_.driver(); }\n> >> @@ -152,6 +158,9 @@ protected:\n> >>  \tstd::string logPrefix() const;\n> >>  \n> >>  private:\n> >> +\tint queryBufferType();\n> >> +\tint queryBufferType(enum v4l2_buf_type type);\n> > \n> > I think these are leftovers.\n> \n> Yes, removed.\n> \n> >> +\n> >>  \tint getFormatMeta(V4L2DeviceFormat *format);\n> >>  \tint setFormatMeta(V4L2DeviceFormat *format);\n> >>  \n> >> @@ -182,6 +191,25 @@ private:\n> >>  \tEventNotifier *fdEvent_;\n> >>  };\n> >>  \n> >> +class V4L2M2MDevice\n> >> +{\n> >> +public:\n> >> +\tV4L2M2MDevice(const std::string &deviceNode);\n> >> +\t~V4L2M2MDevice();\n> >> +\n> >> +\tint open();\n> >> +\tvoid close();\n> >> +\n> >> +\tV4L2VideoDevice *output() { return output_; }\n> >> +\tV4L2VideoDevice *capture() { return capture_; }\n> >> +\n> >> +private:\n> >> +\tstd::string deviceNode_;\n> >> +\n> >> +\tV4L2VideoDevice *output_;\n> >> +\tV4L2VideoDevice *capture_;\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..9c5638995577 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> >> @@ -296,6 +302,10 @@ V4L2VideoDevice::~V4L2VideoDevice()\n> >>  \n> >>  /**\n> >>   * \\brief Open a V4L2 video device and query its capabilities\n> >> + *\n> >> + * Opens a video device, then queries the capabilities of the device and\n> > \n> > s/Opens/This method opens/\n> > \n> >> + * establishes the buffer types and device events accordingly.\n> > \n> > The last part is internal matters I think. I would just drop this\n> > paragraph, and update the brief to state \"Open a V4L2 video device node\n> > and query its capabilities\".\n> \n> You mean simply add the word 'node' to the existing brief?\n\nYes, as the main difference between this method and the fd-based open\nmethod is that this one operates on a device node.\n\n> The 'a' doesn't give enough context in that case any more (or perhaps\n> before), because it only opens 'the' device node that exists in this\n> class instance. It doesn't open any device node. Only the one which was\n> previously set in the constructor.\n> \n> I'll change it to :\n> \n>   \"open /the/ V4L2 video device node and ...\"\n\nSounds good to me.\n\n> >> + *\n> >>   * \\return 0 on success or a negative error code otherwise\n> >>   */\n> >>  int V4L2VideoDevice::open()\n> >> @@ -355,6 +365,83 @@ int V4L2VideoDevice::open()\n> >>  \treturn 0;\n> >>  }\n> >>  \n> >> +/**\n> >> + * \\brief Open a V4L2 video device and query its capabilities\n> > \n> > And here \"Open a V4L2 video device from an opened file handle and query\n> > its capabilities\".\n> \n> Changed.\n> \n> >> + *\n> > \n> > No need for this blank line.\n> > \n> >> + * \\param[in] handle The file descriptor to set\n> > \n> > s/handle/fd/ ?\n> \n> No,.\n> \n> >> + * \\param[in] type The device type to operate on\n> >> + *\n> >> + * Sets the file descriptor for the device and then queries the capabilities of\n> > \n> > s/Sets/This method sets/\n> > \n> > You need a subject before a conjugated verb.\n> \n> We're /in/ the subject. It can be implicit.\n\nIt could just be me, and my over-exposure to pesky French grammar and\nconjugation :-) I've always been taught that a sentence that contain a\ncnjugated verb at any mood other than the imperative requires an\nexplicit subject. Are the rules different in English ?\n\nInterestingly enough the style of the \\brief seems fine to me, even\nthough I'm not sure why. \"Open a V4L2 video device\" sounds correct to my\nears (but what tense is \"open\" conjugated in ?), while starting a\nsentence with \"Opens a V4L2 video device\" sounds like something is\nmissing.\n\nI'm obviously not challenging your native English knowledge :-) I would\nhowever like to understand what the grammatical rules are, in order to\nimprove my own knowledge (and stop perstering everybody with similar\ngrammatical comments).\n\n> >> + * the device and establishes the buffer types and device events accordingly.\n> > \n> >  * This methods opens a video device from the existing file descriptor \\a fd.\n> \n> We can't use fd as an argument, because we use fd().\n\nAh yes, I didn't notice that. We could use the qualified\nV4L2Device::fd() version and name the argument fd, but it's not worth\nit. Let's thus keep handle.\n\n> We can't use newFd, because that's the result of the dup().\n> \n> That's why I've used handle.\n> \n> >  * Like open() queries the capabilities of the device, but handles it according\n> \n>                 ^\n> Isn't this the same issue?\n\nAbsolutely :-) Sorry for the mistake.\n\n> \n> >  * to the given device \\a type instead of determining its type from the\n> >  * capabilities. This can be used to force a given device type for M2M devices.\n> >  *\n> >  * The file descriptor \\a fd is duplicated, and the caller shall close \\a fd\n> >  * when it has no further use for it. The close() method will close the\n> >  * duplicated file descriptor, leaving \\a fd untouched.\n> \n> Changed to:\n> \n>  * This methods opens a video device from the existing file descriptor \\a\n\ns/methods/method/ ?\n\n>  * handle. Like open(), this method queries the capabilities of the device, but\n\nI would have written s/this method/it/ but that's up to you.\n\n>  * handles it according to the given device \\a type instead of determining its\n>  * type from the capabilities. This can be used to force a given device type for\n>  * M2M devices.\n\nSounds very good to me.\n\n>  *\n>  * The file descriptor \\a handle is duplicated, and the caller is responsible\n>  * for closing the \\a handle when it has no further use for it. The close()\n>  * method will close the duplicated file descriptor, leaving \\a handle\n>  * untouched.\n> \n> >> + *\n> >> + * \\return 0 on success or a negative error code otherwise\n> >> + */\n> >> +int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> >> +{\n> >> +\tint ret;\n> >> +\tint newFd;\n> >> +\n> >> +\tnewFd = dup(handle);\n> >> +\tif (newFd < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> >> +\t\t\t\t << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tret = V4L2Device::setFd(newFd);\n> >> +\tif (ret < 0) {\n> >> +\t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n> >> +\t\t\t\t << strerror(-ret);\n> > \n> > You need to ::close(newFd) here.\n> > \n> \n> Ah yes.\n> \n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> >> +\tif (ret < 0) {\n> >> +\t\tLOG(V4L2, Error)\n> >> +\t\t\t<< \"Failed to query device capabilities: \"\n> >> +\t\t\t<< strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tif (!caps_.hasStreaming()) {\n> >> +\t\tLOG(V4L2, Error) << \"Device does not support streaming I/O\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\t/*\n> >> +\t * Set buffer type and wait for read notifications on CAPTURE video\n> >> +\t * devices (POLLIN), and write notifications for OUTPUT video devices\n> >> +\t * (POLLOUT).\n> >> +\t */\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> >> +\tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> >> +\tfdEvent_->setEnabled(false);\n> >> +\n> >> +\tLOG(V4L2, Debug)\n> >> +\t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> >> +\t\t<< caps_.driver() << \": \" << caps_.card();\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >>  /**\n> >>   * \\brief Close the video device, releasing any resources acquired by open()\n> >>   */\n> >> @@ -1143,4 +1230,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> >>  \treturn new V4L2VideoDevice(mediaEntity);\n> >>  }\n> >>  \n> >> +/**\n> >> + * \\class V4L2M2MDevice\n> >> + * \\brief Memory2Memory video device\n> >> + *\n> >> + * The V4L2M2MDevice manages two V4L2VideoDevice instances on the same\n> >> + * deviceNode which operate together using two queues to implement the V4L2\n> >> + * Memory to Memory API.\n> >> + *\n> >> + * The two devices should be opened by calling open() on the V4L2M2MDevice, and\n> >> + * can be closed by calling close on the V4L2M2MDevice.\n> >> + *\n> >> + * Calling V4L2VideoDevice::open() and V4L2VideoDevice::close() on the capture\n> >> + * or output V4L2VideoDevice is not permitted.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::output\n> >> + * \\brief Provide access to the output V4L2VideoDevice instance\n> > \n> > s/Provide access to/Retrieve/\n> \n> Changed.\n> \n> >> + * \\return The output V4L2Device instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MDevice::capture\n> >> + * \\brief Provide access to the capture V4L2VideoDevice instance\n> > \n> > s/Provide access to/Retrieve/\n> \n> Changed.\n> \n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> Thanks,\n> \n> >> + * \\return The capture V4L2Device instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n> >> + */\n> >> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n> >> +\t: deviceNode_(deviceNode)\n> >> +{\n> >> +\toutput_ = new V4L2VideoDevice(deviceNode);\n> >> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n> >> +}\n> >> +\n> >> +V4L2M2MDevice::~V4L2M2MDevice()\n> >> +{\n> >> +\tdelete capture_;\n> >> +\tdelete output_;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Open a V4L2 Memory to Memory device\n> >> + *\n> >> + * Open the device node and prepare the two V4L2VideoDevice instances to handle\n> >> + * their respective buffer queues.\n> >> + *\n> >> + * \\return 0 on success or a negative error code otherwise\n> >> + */\n> >> +int V4L2M2MDevice::open()\n> >> +{\n> >> +\tint fd;\n> >> +\tint ret;\n> >> +\n> >> +\t/*\n> >> +\t * The output and capture V4L2VideoDevice instances use the same file\n> >> +\t * handle for the same device node. The local file handle can be closed\n> >> +\t * as the V4L2VideoDevice::open() retains a handle by duplicating the\n> >> +\t * fd passed in.\n> >> +\t */\n> >> +\tfd = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);\n> >> +\tif (fd < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2, Error)\n> >> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> >> +\tif (ret)\n> >> +\t\tgoto err;\n> >> +\n> >> +\tret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> >> +\tif (ret)\n> >> +\t\tgoto err;\n> >> +\n> >> +\t::close(fd);\n> >> +\n> >> +\treturn 0;\n> >> +\n> >> +err:\n> >> +\tclose();\n> >> +\t::close(fd);\n> >> +\n> >> +\treturn ret;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Close the memory-to-memory device, releasing any resources acquired by\n> >> + * open()\n> >> + */\n> >> +void V4L2M2MDevice::close()\n> >> +{\n> >> +\tcapture_->close();\n> >> +\toutput_->close();\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 291AF61582\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 12 Aug 2019 13:16:40 +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 95491327;\n\tMon, 12 Aug 2019 13:16:39 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565608599;\n\tbh=cS/+FbIllC7FA84fY/M5bEDhdvPJVY2FJbEWEi6HlPs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PedeqmvQeAdMs318N2j2wvfvwoOQWfmP3QyUbpvw3rmfjifLCVgHED2RlLwq0AC96\n\tXHoIpbfKYCefSZm05B/dfazRwxrLp3jj7D3Sk3n2VGrSWw4JNjNbwBznZfsLud0nbo\n\tNIL/u8RUVhL40kNgo6y8t3P0jMTWMuVNaEP1M/Q0=","Date":"Mon, 12 Aug 2019 14:16:37 +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":"<20190812111637.GE5006@pendragon.ideasonboard.com>","References":"<20190809150459.14421-1-kieran.bingham@ideasonboard.com>\n\t<20190809150459.14421-4-kieran.bingham@ideasonboard.com>\n\t<20190810134110.GA4788@pendragon.ideasonboard.com>\n\t<3c82e353-5d68-cc3d-461d-aa8b4d05eedf@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<3c82e353-5d68-cc3d-461d-aa8b4d05eedf@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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":"Mon, 12 Aug 2019 11:16:40 -0000"}},{"id":2392,"web_url":"https://patchwork.libcamera.org/comment/2392/","msgid":"<7dd7778a-8167-c71e-5d44-67d44e4fc204@ideasonboard.com>","date":"2019-08-12T12:42:30","subject":"Re: [libcamera-devel] [PATCH v2 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 12/08/2019 12:16, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Aug 12, 2019 at 10:01:53AM +0100, Kieran Bingham wrote:\n>> On 10/08/2019 14:41, Laurent Pinchart wrote:\n>>> On Fri, Aug 09, 2019 at 04:04:56PM +0100, Kieran Bingham wrote:\n>>>> V4L2 M2M devices represent a V4L2Device with two queues: One output, and\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 |  28 ++++\n>>>>  src/libcamera/v4l2_videodevice.cpp       | 186 +++++++++++++++++++++++\n>>>>  2 files changed, 214 insertions(+)\n>>>>\n>>>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n>>>> index f5c8da93fcb5..72dc8c63e4bb 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>>>> @@ -124,6 +129,7 @@ public:\n>>>>  \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n>>>>  \n>>>>  \tint open();\n>>>> +\tint open(int handle, enum v4l2_buf_type type);\n>>>>  \tvoid close();\n>>>>  \n>>>>  \tconst char *driverName() const { return caps_.driver(); }\n>>>> @@ -152,6 +158,9 @@ protected:\n>>>>  \tstd::string logPrefix() const;\n>>>>  \n>>>>  private:\n>>>> +\tint queryBufferType();\n>>>> +\tint queryBufferType(enum v4l2_buf_type type);\n>>>\n>>> I think these are leftovers.\n>>\n>> Yes, removed.\n>>\n>>>> +\n>>>>  \tint getFormatMeta(V4L2DeviceFormat *format);\n>>>>  \tint setFormatMeta(V4L2DeviceFormat *format);\n>>>>  \n>>>> @@ -182,6 +191,25 @@ private:\n>>>>  \tEventNotifier *fdEvent_;\n>>>>  };\n>>>>  \n>>>> +class V4L2M2MDevice\n>>>> +{\n>>>> +public:\n>>>> +\tV4L2M2MDevice(const std::string &deviceNode);\n>>>> +\t~V4L2M2MDevice();\n>>>> +\n>>>> +\tint open();\n>>>> +\tvoid close();\n>>>> +\n>>>> +\tV4L2VideoDevice *output() { return output_; }\n>>>> +\tV4L2VideoDevice *capture() { return capture_; }\n>>>> +\n>>>> +private:\n>>>> +\tstd::string deviceNode_;\n>>>> +\n>>>> +\tV4L2VideoDevice *output_;\n>>>> +\tV4L2VideoDevice *capture_;\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..9c5638995577 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>>>> @@ -296,6 +302,10 @@ V4L2VideoDevice::~V4L2VideoDevice()\n>>>>  \n>>>>  /**\n>>>>   * \\brief Open a V4L2 video device and query its capabilities\n>>>> + *\n>>>> + * Opens a video device, then queries the capabilities of the device and\n>>>\n>>> s/Opens/This method opens/\n>>>\n>>>> + * establishes the buffer types and device events accordingly.\n>>>\n>>> The last part is internal matters I think. I would just drop this\n>>> paragraph, and update the brief to state \"Open a V4L2 video device node\n>>> and query its capabilities\".\n>>\n>> You mean simply add the word 'node' to the existing brief?\n> \n> Yes, as the main difference between this method and the fd-based open\n> method is that this one operates on a device node.\n> \n>> The 'a' doesn't give enough context in that case any more (or perhaps\n>> before), because it only opens 'the' device node that exists in this\n>> class instance. It doesn't open any device node. Only the one which was\n>> previously set in the constructor.\n>>\n>> I'll change it to :\n>>\n>>   \"open /the/ V4L2 video device node and ...\"\n> \n> Sounds good to me.\n> \n>>>> + *\n>>>>   * \\return 0 on success or a negative error code otherwise\n>>>>   */\n>>>>  int V4L2VideoDevice::open()\n>>>> @@ -355,6 +365,83 @@ int V4L2VideoDevice::open()\n>>>>  \treturn 0;\n>>>>  }\n>>>>  \n>>>> +/**\n>>>> + * \\brief Open a V4L2 video device and query its capabilities\n>>>\n>>> And here \"Open a V4L2 video device from an opened file handle and query\n>>> its capabilities\".\n>>\n>> Changed.\n>>\n>>>> + *\n>>>\n>>> No need for this blank line.\n>>>\n>>>> + * \\param[in] handle The file descriptor to set\n>>>\n>>> s/handle/fd/ ?\n>>\n>> No,.\n>>\n>>>> + * \\param[in] type The device type to operate on\n>>>> + *\n>>>> + * Sets the file descriptor for the device and then queries the capabilities of\n>>>\n>>> s/Sets/This method sets/\n>>>\n>>> You need a subject before a conjugated verb.\n>>\n>> We're /in/ the subject. It can be implicit.\n> \n> It could just be me, and my over-exposure to pesky French grammar and\n> conjugation :-) I've always been taught that a sentence that contain a\n> cnjugated verb at any mood other than the imperative requires an\n> explicit subject. Are the rules different in English ?\n\nMaybe we need someone who's taken languages as a degree for that.\n\n<checks with Mrs B>\n\nWell Mrs-B agrees with you, saying it should be at least \"This sets\"\n(with the 'method' being optional)\n\n\n> Interestingly enough the style of the \\brief seems fine to me, even\n> though I'm not sure why. \"Open a V4L2 video device\" sounds correct to my\n> ears (but what tense is \"open\" conjugated in ?), while starting a\n\nOpen is not conjugated - it's just the imperative form.\n\n\n> sentence with \"Opens a V4L2 video device\" sounds like something is\n> missing.\n> \n> I'm obviously not challenging your native English knowledge :-) I would\n> however like to understand what the grammatical rules are, in order to\n> improve my own knowledge (and stop perstering everybody with similar\n> grammatical comments).\n\nWell, I don't think my native language processing follows written rules,\nit just goes with what sounds right :-D ... and provides the subject\nfrom the context - so \"Opens a V4L2 video device\" sounds right to me\n(when in the context of a description of a function).\n\nBut if it's not right - then it's not right.\n\n\n>>>> + * the device and establishes the buffer types and device events accordingly.\n>>>\n>>>  * This methods opens a video device from the existing file descriptor \\a fd.\n>>\n>> We can't use fd as an argument, because we use fd().\n> \n> Ah yes, I didn't notice that. We could use the qualified\n> V4L2Device::fd() version and name the argument fd, but it's not worth\n> it. Let's thus keep handle.\n\nThat's possibly a good option too, but I do like the clarity of having a\ndistinct name.\n\n\n>> We can't use newFd, because that's the result of the dup().\n>>\n>> That's why I've used handle.\n>>\n>>>  * Like open() queries the capabilities of the device, but handles it according\n>>\n>>                 ^\n>> Isn't this the same issue?\n> \n> Absolutely :-) Sorry for the mistake.\n> \n>>\n>>>  * to the given device \\a type instead of determining its type from the\n>>>  * capabilities. This can be used to force a given device type for M2M devices.\n>>>  *\n>>>  * The file descriptor \\a fd is duplicated, and the caller shall close \\a fd\n>>>  * when it has no further use for it. The close() method will close the\n>>>  * duplicated file descriptor, leaving \\a fd untouched.\n>>\n>> Changed to:\n>>\n>>  * This methods opens a video device from the existing file descriptor \\a\n> \n> s/methods/method/ ?\n\nGood spot - fixed.\n\n\n\n>>  * handle. Like open(), this method queries the capabilities of the device, but\n> \n> I would have written s/this method/it/ but that's up to you.\n\nI started with 'it', but it seemed we may as well fully-qualify the subject.\n\nMaybe I'll drop the 'method', and keep \"This queries,' to reduce the\nrepetition.\n\n\n\n\n> \n>>  * handles it according to the given device \\a type instead of determining its\n>>  * type from the capabilities. This can be used to force a given device type for\n>>  * M2M devices.\n> \n> Sounds very good to me.\n> \n>>  *\n>>  * The file descriptor \\a handle is duplicated, and the caller is responsible\n>>  * for closing the \\a handle when it has no further use for it. The close()\n>>  * method will close the duplicated file descriptor, leaving \\a handle\n>>  * untouched.\n>>\n>>>> + *\n>>>> + * \\return 0 on success or a negative error code otherwise\n>>>> + */\n>>>> +int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n>>>> +{\n>>>> +\tint ret;\n>>>> +\tint newFd;\n>>>> +\n>>>> +\tnewFd = dup(handle);\n>>>> +\tif (newFd < 0) {\n>>>> +\t\tret = -errno;\n>>>> +\t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n>>>> +\t\t\t\t << strerror(-ret);\n>>>> +\t\treturn ret;\n>>>> +\t}\n>>>> +\n>>>> +\tret = V4L2Device::setFd(newFd);\n>>>> +\tif (ret < 0) {\n>>>> +\t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n>>>> +\t\t\t\t << strerror(-ret);\n>>>\n>>> You need to ::close(newFd) here.\n>>>\n>>\n>> Ah yes.\n>>\n>>>> +\t\treturn ret;\n>>>> +\t}\n>>>> +\n>>>> +\tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n>>>> +\tif (ret < 0) {\n>>>> +\t\tLOG(V4L2, Error)\n>>>> +\t\t\t<< \"Failed to query device capabilities: \"\n>>>> +\t\t\t<< strerror(-ret);\n>>>> +\t\treturn ret;\n>>>> +\t}\n>>>> +\n>>>> +\tif (!caps_.hasStreaming()) {\n>>>> +\t\tLOG(V4L2, Error) << \"Device does not support streaming I/O\";\n>>>> +\t\treturn -EINVAL;\n>>>> +\t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * Set buffer type and wait for read notifications on CAPTURE video\n>>>> +\t * devices (POLLIN), and write notifications for OUTPUT video devices\n>>>> +\t * (POLLOUT).\n>>>> +\t */\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>>>> +\tfdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n>>>> +\tfdEvent_->setEnabled(false);\n>>>> +\n>>>> +\tLOG(V4L2, Debug)\n>>>> +\t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n>>>> +\t\t<< caps_.driver() << \": \" << caps_.card();\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>>  /**\n>>>>   * \\brief Close the video device, releasing any resources acquired by open()\n>>>>   */\n>>>> @@ -1143,4 +1230,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>>>>  \treturn new V4L2VideoDevice(mediaEntity);\n>>>>  }\n>>>>  \n>>>> +/**\n>>>> + * \\class V4L2M2MDevice\n>>>> + * \\brief Memory2Memory video device\n>>>> + *\n>>>> + * The V4L2M2MDevice manages two V4L2VideoDevice instances on the same\n>>>> + * deviceNode which operate together using two queues to implement the V4L2\n>>>> + * Memory to Memory API.\n>>>> + *\n>>>> + * The two devices should be opened by calling open() on the V4L2M2MDevice, and\n>>>> + * can be closed by calling close on the V4L2M2MDevice.\n>>>> + *\n>>>> + * Calling V4L2VideoDevice::open() and V4L2VideoDevice::close() on the capture\n>>>> + * or output V4L2VideoDevice is not permitted.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2M2MDevice::output\n>>>> + * \\brief Provide access to the output V4L2VideoDevice instance\n>>>\n>>> s/Provide access to/Retrieve/\n>>\n>> Changed.\n>>\n>>>> + * \\return The output V4L2Device instance\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2M2MDevice::capture\n>>>> + * \\brief Provide access to the capture V4L2VideoDevice instance\n>>>\n>>> s/Provide access to/Retrieve/\n>>\n>> Changed.\n>>\n>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>\n>> Thanks,\n>>\n>>>> + * \\return The capture V4L2Device instance\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Create a new V4L2M2MDevice from the \\a deviceNode\n>>>> + */\n>>>> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)\n>>>> +\t: deviceNode_(deviceNode)\n>>>> +{\n>>>> +\toutput_ = new V4L2VideoDevice(deviceNode);\n>>>> +\tcapture_ = new V4L2VideoDevice(deviceNode);\n>>>> +}\n>>>> +\n>>>> +V4L2M2MDevice::~V4L2M2MDevice()\n>>>> +{\n>>>> +\tdelete capture_;\n>>>> +\tdelete output_;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Open a V4L2 Memory to Memory device\n>>>> + *\n>>>> + * Open the device node and prepare the two V4L2VideoDevice instances to handle\n>>>> + * their respective buffer queues.\n>>>> + *\n>>>> + * \\return 0 on success or a negative error code otherwise\n>>>> + */\n>>>> +int V4L2M2MDevice::open()\n>>>> +{\n>>>> +\tint fd;\n>>>> +\tint ret;\n>>>> +\n>>>> +\t/*\n>>>> +\t * The output and capture V4L2VideoDevice instances use the same file\n>>>> +\t * handle for the same device node. The local file handle can be closed\n>>>> +\t * as the V4L2VideoDevice::open() retains a handle by duplicating the\n>>>> +\t * fd passed in.\n>>>> +\t */\n>>>> +\tfd = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);\n>>>> +\tif (fd < 0) {\n>>>> +\t\tret = -errno;\n>>>> +\t\tLOG(V4L2, Error)\n>>>> +\t\t\t<< \"Failed to open V4L2 M2M device: \" << strerror(-ret);\n>>>> +\t\treturn ret;\n>>>> +\t}\n>>>> +\n>>>> +\tret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);\n>>>> +\tif (ret)\n>>>> +\t\tgoto err;\n>>>> +\n>>>> +\tret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);\n>>>> +\tif (ret)\n>>>> +\t\tgoto err;\n>>>> +\n>>>> +\t::close(fd);\n>>>> +\n>>>> +\treturn 0;\n>>>> +\n>>>> +err:\n>>>> +\tclose();\n>>>> +\t::close(fd);\n>>>> +\n>>>> +\treturn ret;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Close the memory-to-memory device, releasing any resources acquired by\n>>>> + * open()\n>>>> + */\n>>>> +void V4L2M2MDevice::close()\n>>>> +{\n>>>> +\tcapture_->close();\n>>>> +\toutput_->close();\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 B68C860E38\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 12 Aug 2019 14:42:34 +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 E3B3C327;\n\tMon, 12 Aug 2019 14:42:33 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565613754;\n\tbh=Fqu/MXHDTahrR8UhOtbtZ+zpaFfXFU1yKgjYgLXFMrI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=MUovRrgzSx7Kl0JIzcLZASI+TsrX3w+iYhiBrEmP+wB3Uumf2QN3K6Li8z/I0X9n0\n\ti7shBx6WT5gww37clSyqZakI/33nPTAD0FMHlrTTfvIOi0i9QYdymsUIDqgnx9jWMa\n\tncMGqszJI6boylk/s/1WqRWuksHBibXa8eDdd7pk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190809150459.14421-1-kieran.bingham@ideasonboard.com>\n\t<20190809150459.14421-4-kieran.bingham@ideasonboard.com>\n\t<20190810134110.GA4788@pendragon.ideasonboard.com>\n\t<3c82e353-5d68-cc3d-461d-aa8b4d05eedf@ideasonboard.com>\n\t<20190812111637.GE5006@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":"<7dd7778a-8167-c71e-5d44-67d44e4fc204@ideasonboard.com>","Date":"Mon, 12 Aug 2019 13:42:30 +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":"<20190812111637.GE5006@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 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":"Mon, 12 Aug 2019 12:42:34 -0000"}}]