[{"id":1876,"web_url":"https://patchwork.libcamera.org/comment/1876/","msgid":"","date":"2019-06-12T13:50:44","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: Introduce\n\tV4L2Device base class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 12/06/2019 14:38, Jacopo Mondi wrote:\n> he V4L2 devices and subdevices share a few common operations,like\n\ns/he/The/\n\n> opening and closing a device node, and perform IOCTLs on the device.\n> \n> With the forthcoming introduction of support for V4L2 controls, the\n> quantity of shared code will drastically increase, as the control\n> implementation is identical for the two.\n> \n> To maximize code re-use and avoid duplications, provide a V4L2Device\n> base class which groups the common operations and members.\n> \n> The newly introduced base class provides methods to open/close a device\n> node, access the file descriptor, and perform IOCTLs on the device.\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Kieran Bingham \n\n\nThank you - this is much easier to review now :-D\n\n\nA few comments below:\n\n> ---\n> src/libcamera/include/v4l2_device.h | 36 ++++++\n> src/libcamera/include/v4l2_subdevice.h | 6 +-\n> src/libcamera/include/v4l2_videodevice.h | 5 +-\n> src/libcamera/meson.build | 2 +\n> src/libcamera/v4l2_device.cpp | 148 +++++++++++++++++++++++\n> src/libcamera/v4l2_subdevice.cpp | 66 +++-------\n> src/libcamera/v4l2_videodevice.cpp | 68 ++++-------\n> 7 files changed, 227 insertions(+), 104 deletions(-)\n> create mode 100644 src/libcamera/include/v4l2_device.h\n> create mode 100644 src/libcamera/v4l2_device.cpp\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> new file mode 100644\n> index 000000000000..584a570a4703\n> --- /dev/null\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -0,0 +1,36 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_device.h - Common base for V4L2 video devices and subdevices\n> + */\n> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device\n> +{\n> +public:\n> +\tvirtual ~V4L2Device() { close(); }\n> +\n> +protected:\n> +\tV4L2Device();\n> +\n> +\tint fd() { return fd_; }\n> +\n> +\tint open(const std::string &pathname, unsigned int flags);\n> +\tint close();\n> +\tbool isOpen() const;\n> +\n> +\tint ioctl(unsigned long request, void *argp);\n> +\n> +private:\n> +\tint fd_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 3e4e5107aebe..a34b719f8c52 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -16,6 +16,7 @@\n> #include \"formats.h\"\n> #include \"log.h\"\n> #include \"media_object.h\"\n> +#include \"v4l2_device.h\"\n> \n> namespace libcamera {\n> \n> @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {\n> \tconst std::string toString() const;\n> };\n> \n> -class V4L2Subdevice : protected Loggable\n> +class V4L2Subdevice : public V4L2Device, protected Loggable\n> {\n> public:\n> \texplicit V4L2Subdevice(const MediaEntity *entity);\n> @@ -37,8 +38,6 @@ public:\n> \t~V4L2Subdevice();\n> \n> \tint open();\n> -\tbool isOpen() const;\n> -\tvoid close();\n> \n> \tconst MediaEntity *entity() const { return entity_; }\n> \n> @@ -64,7 +63,6 @@ private:\n> \t\t\t Rectangle *rect);\n> \n> \tconst MediaEntity *entity_;\n> -\tint fd_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index 6ecdb64e5f3c..15a83635b9bf 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \"log.h\"\n> +#include \"v4l2_device.h\"\n> \n> namespace libcamera {\n> \n> @@ -111,7 +112,7 @@ public:\n> \tconst std::string toString() const;\n> };\n> \n> -class V4L2VideoDevice : protected Loggable\n> +class V4L2VideoDevice : public V4L2Device, protected Loggable\n> {\n> public:\n> \texplicit V4L2VideoDevice(const std::string &deviceNode);\n> @@ -122,7 +123,6 @@ public:\n> \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n> \n> \tint open();\n> -\tbool isOpen() const;\n> \tvoid close();\n> \n> \tconst char *driverName() const { return caps_.driver(); }\n> @@ -167,7 +167,6 @@ private:\n> \tvoid bufferAvailable(EventNotifier *notifier);\n> \n> \tstd::string deviceNode_;\n> -\tint fd_;\n> \tV4L2Capability caps_;\n> \n> \tenum v4l2_buf_type bufferType_;\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 15ab53b1abbe..f26ad5b2dc57 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -23,6 +23,7 @@ libcamera_sources = files([\n> 'stream.cpp',\n> 'timer.cpp',\n> 'utils.cpp',\n> + 'v4l2_device.cpp',\n> 'v4l2_subdevice.cpp',\n> 'v4l2_videodevice.cpp',\n> ])\n> @@ -41,6 +42,7 @@ libcamera_headers = files([\n> 'include/media_object.h',\n> 'include/pipeline_handler.h',\n> 'include/utils.h',\n> + 'include/v4l2_device.h',\n> 'include/v4l2_subdevice.h',\n> 'include/v4l2_videodevice.h',\n> ])\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> new file mode 100644\n> index 000000000000..f89b2a2982ba\n> --- /dev/null\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -0,0 +1,148 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_device.cpp - Common base for V4L2 devices and subdevices\n> + */\n> +\n> +#include \"v4l2_device.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file v4l2_device.h\n> + * \\brief Common base for V4L2 devices and subdevices\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(V4L2)\n> +\n> +/**\n> + * \\class V4L2Device\n> + * \\brief Base class for V4L2Videodev and V4L2Subdev\n> + *\n> + * The V4L2Device class groups together the methods and fields common to\n> + * both V4L2Videodev and V4L2Subdev, and provide a base class which\n> + * provides methods to open and close the device node associated with the\n> + * device and to perform IOCTL system calls on it.\n> + *\n> + * The V4L2Device class cannot be instantiated directly, as its constructor\n> + * is protected. Users should use instead one the derived classes to model\n> + * either a V4L2 video device or a V4L2 subdevice.\n> + */\n> +\n> +/**\n> + * \\brief Construct a V4L2Device\n> + *\n> + * The V4L2Device constructor is protected and can only be accessed by the\n> + * V4L2Videodev and V4L2Subdev derived classes.\n> + *\n> + * Initialize the file descriptor to -1.\n> + */\n> +V4L2Device::V4L2Device()\n> +\t: fd_(-1)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn V4L2Device::fd()\n> + * \\brief Retrieve the V4L2 device file descriptor\n> + * \\return The V4L2 device file descriptor, -1 if the device node is not open\n> + */\n> +\n> +/**\n> + * \\brief Open a V4L2 device node\n> + * \\param pathname The filesystem path of the device node to open\n> + * \\param flags Access mode flags\n> + *\n> + * Initialize the file descriptor, which was initially set to -1.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Device::open(const std::string &pathname, unsigned int flags)\n> +{\n> +\tif (isOpen()) {\n> +\t\tLOG(V4L2, Error) << \"Device already open\";\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n> +\tint ret = ::open(pathname.c_str(), flags);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2, Error) << \"Failed to open V4L2 device: \"\n> +\t\t\t\t << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfd_ = ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Close the device node\n> + *\n> + * Reset the file descriptor to -1\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Device::close()\n> +{\n> +\tif (fd_ < 0)\n> +\t\treturn 0;\n> +\n> +\tint ret = ::close(fd_);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> +\t\t\t\t << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfd_ = -1;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Check if the V4L2 device node is open\n> + * \\return True if the V4L2 device node is open, false otherwise\n> + */\n> +bool V4L2Device::isOpen() const\n> +{\n> +\treturn fd_ != -1;\n> +}\n> +\n> +/**\n> + * \\brief Perform an IOCTL system call on the device node\n> + * \\param request The IOCTL request code\n> + * \\param argp A poiunter to the IOCTL argument\n\n\ns/poiunter/pointer/\n\n\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Device::ioctl(unsigned long request, void *argp)\n> +{\n> +\t/*\n> +\t * Printing out an error message is usually better performed\n> +\t * in the caller, which can provide more context.\n> +\t */\n> +\tif (::ioctl(fd_, request, argp) < 0)\n> +\t\treturn -errno;\n\nI /really/ like this ... but note how many of the call sites for\nV4L2Device::ioctl() still work with the -errno fuss, which should now\njust deal directly with 'ret' where it's used.\n\n'man ioctl' states:\n\nRETURN VALUE\n Usually, on success zero is returned. A few ioctl() requests\n use the return value as an output parameter and return a\n nonnegative value on success. On error, -1 is returned, and\n errno is set appropriately.\n\n\nI really like simplifying the -1, errno check pattern that is\neverywhere. But do we need to be sure we'll never need the return code\nof the ioctl?\n\n(I don't think we do - and we /could/ have a different call for that if\nnecessary)\n\n\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\var V4L2Device::fd_\n> + * \\brief The V4L2 device node file descriptor\n> + *\n> + * The file descriptor is initialized to -1 and reset to this value once\n> + * the device node gets closed.\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 6681c1920065..b1e5f559b944 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -29,7 +29,7 @@\n> \n> namespace libcamera {\n> \n> -LOG_DEFINE_CATEGORY(V4L2Subdev)\n> +LOG_DECLARE_CATEGORY(V4L2)\n> \n> /**\n> * \\struct V4L2SubdeviceFormat\n> @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const\n> * path\n> */\n> V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)\n> -\t: entity_(entity), fd_(-1)\n> +\t: V4L2Device(), entity_(entity)\n> {\n> }\n> \n> @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()\n> */\n> int V4L2Subdevice::open()\n> {\n> -\tint ret;\n> -\n> -\tif (isOpen()) {\n> -\t\tLOG(V4L2Subdev, Error) << \"Device already open\";\n> -\t\treturn -EBUSY;\n> -\t}\n> -\n> -\tret = ::open(entity_->deviceNode().c_str(), O_RDWR);\n> -\tif (ret < 0) {\n> -\t\tret = -errno;\n> -\t\tLOG(V4L2Subdev, Error)\n> -\t\t\t<< \"Failed to open V4L2 subdevice '\"\n> -\t\t\t<< entity_->deviceNode() << \"': \" << strerror(-ret);\n> -\t\treturn ret;\n> -\t}\n> -\tfd_ = ret;\n> -\n> -\treturn 0;\n> -}\n> -\n> -/**\n> - * \\brief Check if the subdevice is open\n> - * \\return True if the subdevice is open, false otherwise\n> - */\n> -bool V4L2Subdevice::isOpen() const\n> -{\n> -\treturn fd_ != -1;\n> -}\n> -\n> -/**\n> - * \\brief Close the subdevice, releasing any resources acquired by open()\n> - */\n> -void V4L2Subdevice::close()\n> -{\n> -\tif (!isOpen())\n> -\t\treturn;\n> -\n> -\t::close(fd_);\n> -\tfd_ = -1;\n> +\treturn V4L2Device::open(entity_->deviceNode(), O_RDWR);\n> }\n> \n> /**\n> @@ -207,7 +169,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)\n> \tint ret;\n> \n> \tif (pad >= entity_->pads().size()) {\n> -\t\tLOG(V4L2Subdev, Error) << \"Invalid pad: \" << pad;\n> +\t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> \t\treturn formatMap;\n> \t}\n> \n> @@ -215,7 +177,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)\n> \tmbusEnum.index = 0;\n> \tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> \twhile (true) {\n> -\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);\n> +\t\tret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);\n> \t\tif (ret)\n> \t\t\tbreak;\n> \n> @@ -229,7 +191,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)\n> \n> \tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> \t\tret = -errno;\n> -\t\tLOG(V4L2Subdev, Error)\n> +\t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> \t\t\t<< \": \" << strerror(-ret);\n> \t\tformatMap.clear();\n> @@ -250,10 +212,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> \tsubdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> \tsubdevFmt.pad = pad;\n> \n> -\tint ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> +\tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> \tif (ret) {\n> \t\tret = -errno;\n> -\t\tLOG(V4L2Subdev, Error)\n> +\t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to get format on pad \" << pad\n> \t\t\t<< \": \" << strerror(-ret);\n> \t\treturn ret;\n> @@ -286,10 +248,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> \tsubdevFmt.format.height = format->size.height;\n> \tsubdevFmt.format.code = format->mbus_code;\n> \n> -\tint ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> +\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> \tif (ret) {\n> \t\tret = -errno;\n> -\t\tLOG(V4L2Subdev, Error)\n> +\t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to set format on pad \" << pad\n> \t\t\t<< \": \" << strerror(-ret);\n> \t\treturn ret;\n> @@ -339,7 +301,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,\n> \tsizeEnum.code = code;\n> \tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> \twhile (true) {\n> -\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);\n> +\t\tret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);\n> \t\tif (ret)\n> \t\t\tbreak;\n> \n> @@ -351,7 +313,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,\n> \n> \tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> \t\tret = -errno;\n> -\t\tLOG(V4L2Subdev, Error)\n> +\t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> \t\t\t<< \": \" << strerror(-ret);\n> \t\tsizes->clear();\n> @@ -377,10 +339,10 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> \tsel.r.width = rect->w;\n> \tsel.r.height = rect->h;\n> \n> -\tint ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);\n> +\tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> -\t\tLOG(V4L2Subdev, Error)\n> +\t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> \t\t\t<< pad << \": \" << strerror(-ret);\n> \t\treturn ret;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 8957cf8f97d3..14e711d5dde3 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -30,7 +30,7 @@\n> */\n> namespace libcamera {\n> \n> -LOG_DEFINE_CATEGORY(V4L2)\n> +LOG_DECLARE_CATEGORY(V4L2)\n> \n> /**\n> * \\struct V4L2Capability\n> @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const\n> * \\param[in] deviceNode The file-system path to the video device node\n> */\n> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),\n> +\t: V4L2Device(), deviceNode_(deviceNode), bufferPool_(nullptr),\n> \t queuedBuffersCount_(0), fdEvent_(nullptr)\n> {\n> \t/*\n> @@ -305,21 +305,11 @@ int V4L2VideoDevice::open()\n> {\n> \tint ret;\n> \n> -\tif (isOpen()) {\n> -\t\tLOG(V4L2, Error) << \"Device already open\";\n> -\t\treturn -EBUSY;\n> -\t}\n> -\n> -\tret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);\n> -\tif (ret < 0) {\n> -\t\tret = -errno;\n> -\t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Failed to open V4L2 device: \" << strerror(-ret);\n> +\tret = V4L2Device::open(deviceNode_, O_RDWR | O_NONBLOCK);\n> +\tif (ret < 0)\n> \t\treturn ret;\n> -\t}\n> -\tfd_ = ret;\n> \n> -\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n> +\tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error)\n> @@ -342,20 +332,20 @@ int V4L2VideoDevice::open()\n> \t * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).\n> \t */\n> \tif (caps_.isVideoCapture()) {\n> -\t\tfdEvent_ = new EventNotifier(fd_, EventNotifier::Read);\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\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\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\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> @@ -368,28 +358,16 @@ int V4L2VideoDevice::open()\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief Check if device is successfully opened\n> - * \\return True if the device is open, false otherwise\n> - */\n> -bool V4L2VideoDevice::isOpen() const\n> -{\n> -\treturn fd_ != -1;\n> -}\n> -\n> /**\n> * \\brief Close the device, releasing any resources acquired by open()\n> */\n> void V4L2VideoDevice::close()\n> {\n> -\tif (fd_ < 0)\n> +\tif (fd() < 0)\n> \t\treturn;\n> \n> \treleaseBuffers();\n> \tdelete fdEvent_;\n> -\n> -\t::close(fd_);\n> -\tfd_ = -1;\n> }\n> \n> /**\n> @@ -462,7 +440,7 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n> \tint ret;\n> \n> \tv4l2Format.type = bufferType_;\n> -\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);\n> +\tret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n\n\nI really like that ioctl now knows about the FD internally and thus is\nsimpler :)\n\n\"do this action with this structure...\"\n\n\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n\nThis is an example of things that I think should now be:\n\nret = ioctl(VIDIOC_G_FMT, &v4l2Format);\nif (ret) {\n\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(ret);\n\t...\n}\n\nThat probably applies to all uses of V4L2Device::ioctl()\n\n\n> @@ -488,7 +466,7 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)\n> \tv4l2Format.type = bufferType_;\n> \tpix->dataformat = format->fourcc;\n> \tpix->buffersize = format->planes[0].size;\n> -\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);\n> +\tret = ioctl(VIDIOC_S_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error) << \"Unable to set format: \" << strerror(-ret);\n> @@ -516,7 +494,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> \tint ret;\n> \n> \tv4l2Format.type = bufferType_;\n> -\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);\n> +\tret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n> @@ -554,7 +532,7 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)\n> \t\tpix->plane_fmt[i].sizeimage = format->planes[i].size;\n> \t}\n> \n> -\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);\n> +\tret = ioctl(VIDIOC_S_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error) << \"Unable to set format: \" << strerror(-ret);\n> @@ -584,7 +562,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> \tint ret;\n> \n> \tv4l2Format.type = bufferType_;\n> -\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);\n> +\tret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n> @@ -613,7 +591,7 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> \tpix->pixelformat = format->fourcc;\n> \tpix->bytesperline = format->planes[0].bpl;\n> \tpix->field = V4L2_FIELD_NONE;\n> -\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);\n> +\tret = ioctl(VIDIOC_S_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error) << \"Unable to set format: \" << strerror(-ret);\n> @@ -643,7 +621,7 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)\n> \trb.type = bufferType_;\n> \trb.memory = memoryType_;\n> \n> -\tret = ioctl(fd_, VIDIOC_REQBUFS, &rb);\n> +\tret = ioctl(VIDIOC_REQBUFS, &rb);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error)\n> @@ -695,7 +673,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n> \t\tbuf.length = VIDEO_MAX_PLANES;\n> \t\tbuf.m.planes = planes;\n> \n> -\t\tret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);\n> +\t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> \t\tif (ret < 0) {\n> \t\t\tret = -errno;\n> \t\t\tLOG(V4L2, Error)\n> @@ -748,7 +726,7 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,\n> \texpbuf.plane = planeIndex;\n> \texpbuf.flags = O_RDWR;\n> \n> -\tret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);\n> +\tret = ioctl(VIDIOC_EXPBUF, &expbuf);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error)\n> @@ -855,7 +833,7 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> \n> \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n> \n> -\tret = ioctl(fd_, VIDIOC_QBUF, &buf);\n> +\tret = ioctl(VIDIOC_QBUF, &buf);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error)\n> @@ -892,7 +870,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> \t\tbuf.m.planes = planes;\n> \t}\n> \n> -\tret = ioctl(fd_, VIDIOC_DQBUF, &buf);\n> +\tret = ioctl(VIDIOC_DQBUF, &buf);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error)\n> @@ -952,7 +930,7 @@ int V4L2VideoDevice::streamOn()\n> {\n> \tint ret;\n> \n> -\tret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);\n> +\tret = ioctl(VIDIOC_STREAMON, &bufferType_);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error)\n> @@ -975,7 +953,7 @@ int V4L2VideoDevice::streamOff()\n> {\n> \tint ret;\n> \n> -\tret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);\n> +\tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error)\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D477A62FB9\n\tfor ;\n\tWed, 12 Jun 2019 15:50:47 +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 3A29C9B1;\n\tWed, 12 Jun 2019 15:50:47 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560347447;\n\tbh=N3m/bLL+mzb8ajKeoXV4dciIhVKyW9JQtmK4Dp02wjo=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=YZ2vlPIRYmNj0BT4MMRjWrERn2gv6uwC0w8ahzbkE2rjwJeIY5dFNMaVYFeFzxeiJ\n\tar9Tx0GI9zai3GCLB+eepHH0sOCEj/G8JfqxaitSaA/LfRJiTsmnNW5Xvsidhuthjn\n\tpoLloffsuaidPz/pAcQDvcCArOIdYl45FA2AClN4=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20190612133817.32731-1-jacopo@jmondi.org>\n\t<20190612133817.32731-3-jacopo@jmondi.org>","From":"Kieran Bingham ","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":"","Date":"Wed, 12 Jun 2019 14:50:44 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190612133817.32731-3-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: Introduce\n\tV4L2Device base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 12 Jun 2019 13:50:48 -0000"}},{"id":1877,"web_url":"https://patchwork.libcamera.org/comment/1877/","msgid":"","date":"2019-06-12T13:51:46","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: Introduce\n\tV4L2Device base class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Oh and,\n\nOn 12/06/2019 14:50, Kieran Bingham wrote:\n> Hi Jacopo,\n> \n> On 12/06/2019 14:38, Jacopo Mondi wrote:\n>> he V4L2 devices and subdevices share a few common operations,like\n> \n> s/he/The/\n> \n>> opening and closing a device node, and perform IOCTLs on the device.\n>>\n>> With the forthcoming introduction of support for V4L2 controls, the\n>> quantity of shared code will drastically increase, as the control\n>> implementation is identical for the two.\n>>\n>> To maximize code re-use and avoid duplications, provide a V4L2Device\n>> base class which groups the common operations and members.\n>>\n>> The newly introduced base class provides methods to open/close a device\n>> node, access the file descriptor, and perform IOCTLs on the device.\n>>\n>> Signed-off-by: Jacopo Mondi \n>> Signed-off-by: Kieran Bingham \n> \n> \n> Thank you - this is much easier to review now :-D\n> \n> \n> A few comments below:\n\nWith comments addressed as you see fit,\n\nReviewed-by: Kieran Bingham \n\n\n> \n>> ---\n>> src/libcamera/include/v4l2_device.h | 36 ++++++\n>> src/libcamera/include/v4l2_subdevice.h | 6 +-\n>> src/libcamera/include/v4l2_videodevice.h | 5 +-\n>> src/libcamera/meson.build | 2 +\n>> src/libcamera/v4l2_device.cpp | 148 +++++++++++++++++++++++\n>> src/libcamera/v4l2_subdevice.cpp | 66 +++-------\n>> src/libcamera/v4l2_videodevice.cpp | 68 ++++-------\n>> 7 files changed, 227 insertions(+), 104 deletions(-)\n>> create mode 100644 src/libcamera/include/v4l2_device.h\n>> create mode 100644 src/libcamera/v4l2_device.cpp\n>>\n>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>> new file mode 100644\n>> index 000000000000..584a570a4703\n>> --- /dev/null\n>> +++ b/src/libcamera/include/v4l2_device.h\n>> @@ -0,0 +1,36 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * v4l2_device.h - Common base for V4L2 video devices and subdevices\n>> + */\n>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n>> +#define __LIBCAMERA_V4L2_DEVICE_H__\n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +class V4L2Device\n>> +{\n>> +public:\n>> +\tvirtual ~V4L2Device() { close(); }\n>> +\n>> +protected:\n>> +\tV4L2Device();\n>> +\n>> +\tint fd() { return fd_; }\n>> +\n>> +\tint open(const std::string &pathname, unsigned int flags);\n>> +\tint close();\n>> +\tbool isOpen() const;\n>> +\n>> +\tint ioctl(unsigned long request, void *argp);\n>> +\n>> +private:\n>> +\tint fd_;\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n>> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n>> index 3e4e5107aebe..a34b719f8c52 100644\n>> --- a/src/libcamera/include/v4l2_subdevice.h\n>> +++ b/src/libcamera/include/v4l2_subdevice.h\n>> @@ -16,6 +16,7 @@\n>> #include \"formats.h\"\n>> #include \"log.h\"\n>> #include \"media_object.h\"\n>> +#include \"v4l2_device.h\"\n>> \n>> namespace libcamera {\n>> \n>> @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {\n>> \tconst std::string toString() const;\n>> };\n>> \n>> -class V4L2Subdevice : protected Loggable\n>> +class V4L2Subdevice : public V4L2Device, protected Loggable\n>> {\n>> public:\n>> \texplicit V4L2Subdevice(const MediaEntity *entity);\n>> @@ -37,8 +38,6 @@ public:\n>> \t~V4L2Subdevice();\n>> \n>> \tint open();\n>> -\tbool isOpen() const;\n>> -\tvoid close();\n>> \n>> \tconst MediaEntity *entity() const { return entity_; }\n>> \n>> @@ -64,7 +63,6 @@ private:\n>> \t\t\t Rectangle *rect);\n>> \n>> \tconst MediaEntity *entity_;\n>> -\tint fd_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n>> index 6ecdb64e5f3c..15a83635b9bf 100644\n>> --- a/src/libcamera/include/v4l2_videodevice.h\n>> +++ b/src/libcamera/include/v4l2_videodevice.h\n>> @@ -17,6 +17,7 @@\n>> #include \n>> \n>> #include \"log.h\"\n>> +#include \"v4l2_device.h\"\n>> \n>> namespace libcamera {\n>> \n>> @@ -111,7 +112,7 @@ public:\n>> \tconst std::string toString() const;\n>> };\n>> \n>> -class V4L2VideoDevice : protected Loggable\n>> +class V4L2VideoDevice : public V4L2Device, protected Loggable\n>> {\n>> public:\n>> \texplicit V4L2VideoDevice(const std::string &deviceNode);\n>> @@ -122,7 +123,6 @@ public:\n>> \tV4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;\n>> \n>> \tint open();\n>> -\tbool isOpen() const;\n>> \tvoid close();\n>> \n>> \tconst char *driverName() const { return caps_.driver(); }\n>> @@ -167,7 +167,6 @@ private:\n>> \tvoid bufferAvailable(EventNotifier *notifier);\n>> \n>> \tstd::string deviceNode_;\n>> -\tint fd_;\n>> \tV4L2Capability caps_;\n>> \n>> \tenum v4l2_buf_type bufferType_;\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 15ab53b1abbe..f26ad5b2dc57 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -23,6 +23,7 @@ libcamera_sources = files([\n>> 'stream.cpp',\n>> 'timer.cpp',\n>> 'utils.cpp',\n>> + 'v4l2_device.cpp',\n>> 'v4l2_subdevice.cpp',\n>> 'v4l2_videodevice.cpp',\n>> ])\n>> @@ -41,6 +42,7 @@ libcamera_headers = files([\n>> 'include/media_object.h',\n>> 'include/pipeline_handler.h',\n>> 'include/utils.h',\n>> + 'include/v4l2_device.h',\n>> 'include/v4l2_subdevice.h',\n>> 'include/v4l2_videodevice.h',\n>> ])\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> new file mode 100644\n>> index 000000000000..f89b2a2982ba\n>> --- /dev/null\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -0,0 +1,148 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * v4l2_device.cpp - Common base for V4L2 devices and subdevices\n>> + */\n>> +\n>> +#include \"v4l2_device.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"log.h\"\n>> +\n>> +/**\n>> + * \\file v4l2_device.h\n>> + * \\brief Common base for V4L2 devices and subdevices\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(V4L2)\n>> +\n>> +/**\n>> + * \\class V4L2Device\n>> + * \\brief Base class for V4L2Videodev and V4L2Subdev\n>> + *\n>> + * The V4L2Device class groups together the methods and fields common to\n>> + * both V4L2Videodev and V4L2Subdev, and provide a base class which\n>> + * provides methods to open and close the device node associated with the\n>> + * device and to perform IOCTL system calls on it.\n>> + *\n>> + * The V4L2Device class cannot be instantiated directly, as its constructor\n>> + * is protected. Users should use instead one the derived classes to model\n>> + * either a V4L2 video device or a V4L2 subdevice.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a V4L2Device\n>> + *\n>> + * The V4L2Device constructor is protected and can only be accessed by the\n>> + * V4L2Videodev and V4L2Subdev derived classes.\n>> + *\n>> + * Initialize the file descriptor to -1.\n>> + */\n>> +V4L2Device::V4L2Device()\n>> +\t: fd_(-1)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\fn V4L2Device::fd()\n>> + * \\brief Retrieve the V4L2 device file descriptor\n>> + * \\return The V4L2 device file descriptor, -1 if the device node is not open\n>> + */\n>> +\n>> +/**\n>> + * \\brief Open a V4L2 device node\n>> + * \\param pathname The filesystem path of the device node to open\n>> + * \\param flags Access mode flags\n>> + *\n>> + * Initialize the file descriptor, which was initially set to -1.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int V4L2Device::open(const std::string &pathname, unsigned int flags)\n>> +{\n>> +\tif (isOpen()) {\n>> +\t\tLOG(V4L2, Error) << \"Device already open\";\n>> +\t\treturn -EBUSY;\n>> +\t}\n>> +\n>> +\tint ret = ::open(pathname.c_str(), flags);\n>> +\tif (ret < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(V4L2, Error) << \"Failed to open V4L2 device: \"\n>> +\t\t\t\t << strerror(-ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tfd_ = ret;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Close the device node\n>> + *\n>> + * Reset the file descriptor to -1\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int V4L2Device::close()\n>> +{\n>> +\tif (fd_ < 0)\n>> +\t\treturn 0;\n>> +\n>> +\tint ret = ::close(fd_);\n>> +\tif (ret < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n>> +\t\t\t\t << strerror(-ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tfd_ = -1;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Check if the V4L2 device node is open\n>> + * \\return True if the V4L2 device node is open, false otherwise\n>> + */\n>> +bool V4L2Device::isOpen() const\n>> +{\n>> +\treturn fd_ != -1;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Perform an IOCTL system call on the device node\n>> + * \\param request The IOCTL request code\n>> + * \\param argp A poiunter to the IOCTL argument\n> \n> \n> s/poiunter/pointer/\n> \n> \n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int V4L2Device::ioctl(unsigned long request, void *argp)\n>> +{\n>> +\t/*\n>> +\t * Printing out an error message is usually better performed\n>> +\t * in the caller, which can provide more context.\n>> +\t */\n>> +\tif (::ioctl(fd_, request, argp) < 0)\n>> +\t\treturn -errno;\n> \n> I /really/ like this ... but note how many of the call sites for\n> V4L2Device::ioctl() still work with the -errno fuss, which should now\n> just deal directly with 'ret' where it's used.\n> \n> 'man ioctl' states:\n> \n> RETURN VALUE\n> Usually, on success zero is returned. A few ioctl() requests\n> use the return value as an output parameter and return a\n> nonnegative value on success. On error, -1 is returned, and\n> errno is set appropriately.\n> \n> \n> I really like simplifying the -1, errno check pattern that is\n> everywhere. But do we need to be sure we'll never need the return code\n> of the ioctl?\n> \n> (I don't think we do - and we /could/ have a different call for that if\n> necessary)\n> \n> \n> \n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\var V4L2Device::fd_\n>> + * \\brief The V4L2 device node file descriptor\n>> + *\n>> + * The file descriptor is initialized to -1 and reset to this value once\n>> + * the device node gets closed.\n>> + */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n>> index 6681c1920065..b1e5f559b944 100644\n>> --- a/src/libcamera/v4l2_subdevice.cpp\n>> +++ b/src/libcamera/v4l2_subdevice.cpp\n>> @@ -29,7 +29,7 @@\n>> \n>> namespace libcamera {\n>> \n>> -LOG_DEFINE_CATEGORY(V4L2Subdev)\n>> +LOG_DECLARE_CATEGORY(V4L2)\n>> \n>> /**\n>> * \\struct V4L2SubdeviceFormat\n>> @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const\n>> * path\n>> */\n>> V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)\n>> -\t: entity_(entity), fd_(-1)\n>> +\t: V4L2Device(), entity_(entity)\n>> {\n>> }\n>> \n>> @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()\n>> */\n>> int V4L2Subdevice::open()\n>> {\n>> -\tint ret;\n>> -\n>> -\tif (isOpen()) {\n>> -\t\tLOG(V4L2Subdev, Error) << \"Device already open\";\n>> -\t\treturn -EBUSY;\n>> -\t}\n>> -\n>> -\tret = ::open(entity_->deviceNode().c_str(), O_RDWR);\n>> -\tif (ret < 0) {\n>> -\t\tret = -errno;\n>> -\t\tLOG(V4L2Subdev, Error)\n>> -\t\t\t<< \"Failed to open V4L2 subdevice '\"\n>> -\t\t\t<< entity_->deviceNode() << \"': \" << strerror(-ret);\n>> -\t\treturn ret;\n>> -\t}\n>> -\tfd_ = ret;\n>> -\n>> -\treturn 0;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Check if the subdevice is open\n>> - * \\return True if the subdevice is open, false otherwise\n>> - */\n>> -bool V4L2Subdevice::isOpen() const\n>> -{\n>> -\treturn fd_ != -1;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Close the subdevice, releasing any resources acquired by open()\n>> - */\n>> -void V4L2Subdevice::close()\n>> -{\n>> -\tif (!isOpen())\n>> -\t\treturn;\n>> -\n>> -\t::close(fd_);\n>> -\tfd_ = -1;\n>> +\treturn V4L2Device::open(entity_->deviceNode(), O_RDWR);\n>> }\n>> \n>> /**\n>> @@ -207,7 +169,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)\n>> \tint ret;\n>> \n>> \tif (pad >= entity_->pads().size()) {\n>> -\t\tLOG(V4L2Subdev, Error) << \"Invalid pad: \" << pad;\n>> +\t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n>> \t\treturn formatMap;\n>> \t}\n>> \n>> @@ -215,7 +177,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)\n>> \tmbusEnum.index = 0;\n>> \tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n>> \twhile (true) {\n>> -\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);\n>> +\t\tret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);\n>> \t\tif (ret)\n>> \t\t\tbreak;\n>> \n>> @@ -229,7 +191,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)\n>> \n>> \tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n>> \t\tret = -errno;\n>> -\t\tLOG(V4L2Subdev, Error)\n>> +\t\tLOG(V4L2, Error)\n>> \t\t\t<< \"Unable to enumerate formats on pad \" << pad\n>> \t\t\t<< \": \" << strerror(-ret);\n>> \t\tformatMap.clear();\n>> @@ -250,10 +212,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n>> \tsubdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n>> \tsubdevFmt.pad = pad;\n>> \n>> -\tint ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n>> +\tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n>> \tif (ret) {\n>> \t\tret = -errno;\n>> -\t\tLOG(V4L2Subdev, Error)\n>> +\t\tLOG(V4L2, Error)\n>> \t\t\t<< \"Unable to get format on pad \" << pad\n>> \t\t\t<< \": \" << strerror(-ret);\n>> \t\treturn ret;\n>> @@ -286,10 +248,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n>> \tsubdevFmt.format.height = format->size.height;\n>> \tsubdevFmt.format.code = format->mbus_code;\n>> \n>> -\tint ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n>> +\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n>> \tif (ret) {\n>> \t\tret = -errno;\n>> -\t\tLOG(V4L2Subdev, Error)\n>> +\t\tLOG(V4L2, Error)\n>> \t\t\t<< \"Unable to set format on pad \" << pad\n>> \t\t\t<< \": \" << strerror(-ret);\n>> \t\treturn ret;\n>> @@ -339,7 +301,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,\n>> \tsizeEnum.code = code;\n>> \tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n>> \twhile (true) {\n>> -\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);\n>> +\t\tret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);\n>> \t\tif (ret)\n>> \t\t\tbreak;\n>> \n>> @@ -351,7 +313,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,\n>> \n>> \tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n>> \t\tret = -errno;\n>> -\t\tLOG(V4L2Subdev, Error)\n>> +\t\tLOG(V4L2, Error)\n>> \t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n>> \t\t\t<< \": \" << strerror(-ret);\n>> \t\tsizes->clear();\n>> @@ -377,10 +339,10 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n>> \tsel.r.width = rect->w;\n>> \tsel.r.height = rect->h;\n>> \n>> -\tint ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);\n>> +\tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> -\t\tLOG(V4L2Subdev, Error)\n>> +\t\tLOG(V4L2, Error)\n>> \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n>> \t\t\t<< pad << \": \" << strerror(-ret);\n>> \t\treturn ret;\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 8957cf8f97d3..14e711d5dde3 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -30,7 +30,7 @@\n>> */\n>> namespace libcamera {\n>> \n>> -LOG_DEFINE_CATEGORY(V4L2)\n>> +LOG_DECLARE_CATEGORY(V4L2)\n>> \n>> /**\n>> * \\struct V4L2Capability\n>> @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const\n>> * \\param[in] deviceNode The file-system path to the video device node\n>> */\n>> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n>> -\t: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),\n>> +\t: V4L2Device(), deviceNode_(deviceNode), bufferPool_(nullptr),\n>> \t queuedBuffersCount_(0), fdEvent_(nullptr)\n>> {\n>> \t/*\n>> @@ -305,21 +305,11 @@ int V4L2VideoDevice::open()\n>> {\n>> \tint ret;\n>> \n>> -\tif (isOpen()) {\n>> -\t\tLOG(V4L2, Error) << \"Device already open\";\n>> -\t\treturn -EBUSY;\n>> -\t}\n>> -\n>> -\tret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);\n>> -\tif (ret < 0) {\n>> -\t\tret = -errno;\n>> -\t\tLOG(V4L2, Error)\n>> -\t\t\t<< \"Failed to open V4L2 device: \" << strerror(-ret);\n>> +\tret = V4L2Device::open(deviceNode_, O_RDWR | O_NONBLOCK);\n>> +\tif (ret < 0)\n>> \t\treturn ret;\n>> -\t}\n>> -\tfd_ = ret;\n>> \n>> -\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n>> +\tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error)\n>> @@ -342,20 +332,20 @@ int V4L2VideoDevice::open()\n>> \t * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).\n>> \t */\n>> \tif (caps_.isVideoCapture()) {\n>> -\t\tfdEvent_ = new EventNotifier(fd_, EventNotifier::Read);\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\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\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\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>> @@ -368,28 +358,16 @@ int V4L2VideoDevice::open()\n>> \treturn 0;\n>> }\n>> \n>> -/**\n>> - * \\brief Check if device is successfully opened\n>> - * \\return True if the device is open, false otherwise\n>> - */\n>> -bool V4L2VideoDevice::isOpen() const\n>> -{\n>> -\treturn fd_ != -1;\n>> -}\n>> -\n>> /**\n>> * \\brief Close the device, releasing any resources acquired by open()\n>> */\n>> void V4L2VideoDevice::close()\n>> {\n>> -\tif (fd_ < 0)\n>> +\tif (fd() < 0)\n>> \t\treturn;\n>> \n>> \treleaseBuffers();\n>> \tdelete fdEvent_;\n>> -\n>> -\t::close(fd_);\n>> -\tfd_ = -1;\n>> }\n>> \n>> /**\n>> @@ -462,7 +440,7 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n>> \tint ret;\n>> \n>> \tv4l2Format.type = bufferType_;\n>> -\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);\n>> +\tret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n> \n> \n> I really like that ioctl now knows about the FD internally and thus is\n> simpler :)\n> \n> \"do this action with this structure...\"\n> \n> \n>> \tif (ret) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n> \n> This is an example of things that I think should now be:\n> \n> ret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n> if (ret) {\n> \tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(ret);\n> \t...\n> }\n> \n> That probably applies to all uses of V4L2Device::ioctl()\n> \n> \n>> @@ -488,7 +466,7 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)\n>> \tv4l2Format.type = bufferType_;\n>> \tpix->dataformat = format->fourcc;\n>> \tpix->buffersize = format->planes[0].size;\n>> -\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);\n>> +\tret = ioctl(VIDIOC_S_FMT, &v4l2Format);\n>> \tif (ret) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error) << \"Unable to set format: \" << strerror(-ret);\n>> @@ -516,7 +494,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n>> \tint ret;\n>> \n>> \tv4l2Format.type = bufferType_;\n>> -\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);\n>> +\tret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n>> \tif (ret) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n>> @@ -554,7 +532,7 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)\n>> \t\tpix->plane_fmt[i].sizeimage = format->planes[i].size;\n>> \t}\n>> \n>> -\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);\n>> +\tret = ioctl(VIDIOC_S_FMT, &v4l2Format);\n>> \tif (ret) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error) << \"Unable to set format: \" << strerror(-ret);\n>> @@ -584,7 +562,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n>> \tint ret;\n>> \n>> \tv4l2Format.type = bufferType_;\n>> -\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);\n>> +\tret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n>> \tif (ret) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n>> @@ -613,7 +591,7 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n>> \tpix->pixelformat = format->fourcc;\n>> \tpix->bytesperline = format->planes[0].bpl;\n>> \tpix->field = V4L2_FIELD_NONE;\n>> -\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);\n>> +\tret = ioctl(VIDIOC_S_FMT, &v4l2Format);\n>> \tif (ret) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error) << \"Unable to set format: \" << strerror(-ret);\n>> @@ -643,7 +621,7 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)\n>> \trb.type = bufferType_;\n>> \trb.memory = memoryType_;\n>> \n>> -\tret = ioctl(fd_, VIDIOC_REQBUFS, &rb);\n>> +\tret = ioctl(VIDIOC_REQBUFS, &rb);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error)\n>> @@ -695,7 +673,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n>> \t\tbuf.length = VIDEO_MAX_PLANES;\n>> \t\tbuf.m.planes = planes;\n>> \n>> -\t\tret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);\n>> +\t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n>> \t\tif (ret < 0) {\n>> \t\t\tret = -errno;\n>> \t\t\tLOG(V4L2, Error)\n>> @@ -748,7 +726,7 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,\n>> \texpbuf.plane = planeIndex;\n>> \texpbuf.flags = O_RDWR;\n>> \n>> -\tret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);\n>> +\tret = ioctl(VIDIOC_EXPBUF, &expbuf);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error)\n>> @@ -855,7 +833,7 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n>> \n>> \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n>> \n>> -\tret = ioctl(fd_, VIDIOC_QBUF, &buf);\n>> +\tret = ioctl(VIDIOC_QBUF, &buf);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error)\n>> @@ -892,7 +870,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n>> \t\tbuf.m.planes = planes;\n>> \t}\n>> \n>> -\tret = ioctl(fd_, VIDIOC_DQBUF, &buf);\n>> +\tret = ioctl(VIDIOC_DQBUF, &buf);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error)\n>> @@ -952,7 +930,7 @@ int V4L2VideoDevice::streamOn()\n>> {\n>> \tint ret;\n>> \n>> -\tret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);\n>> +\tret = ioctl(VIDIOC_STREAMON, &bufferType_);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error)\n>> @@ -975,7 +953,7 @@ int V4L2VideoDevice::streamOff()\n>> {\n>> \tint ret;\n>> \n>> -\tret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);\n>> +\tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n>> \tif (ret < 0) {\n>> \t\tret = -errno;\n>> \t\tLOG(V4L2, Error)\n>>\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA6F462FBC\n\tfor ;\n\tWed, 12 Jun 2019 15:51:48 +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 430D59B1;\n\tWed, 12 Jun 2019 15:51:48 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560347508;\n\tbh=VML/EYUP8id8WBaL2WGSOrZrh2WHd2prhYnUycngyVk=;\n\th=Reply-To:Subject:From:To:References:Date:In-Reply-To:From;\n\tb=Z2QctgW8iOYhP+DylQPR4Bfun23FpheadAEtS6XP88POT7Vq/xy8Q7Az+0H5eviOp\n\tcXsS1BXZf7KtWtcu3uWRKwxe27OAX0T3mVWcmtB1oGzmrU/xm7iN8VR0VTRnQXWRzR\n\t8dXlCoqaCrcKmwT7sb5NNs8CEROOsO3H9jBP+kDU=","Reply-To":"kieran.bingham@ideasonboard.com","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20190612133817.32731-1-jacopo@jmondi.org>\n\t<20190612133817.32731-3-jacopo@jmondi.org>\n\t","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":"","Date":"Wed, 12 Jun 2019 14:51:46 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: Introduce\n\tV4L2Device base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 12 Jun 2019 13:51:49 -0000"}},{"id":1879,"web_url":"https://patchwork.libcamera.org/comment/1879/","msgid":"<20190612163113.ef4ltohhc7ic7kx5@uno.localdomain>","date":"2019-06-12T16:31:13","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: Introduce\n\tV4L2Device base class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Wed, Jun 12, 2019 at 02:50:44PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 12/06/2019 14:38, Jacopo Mondi wrote:\n> > he V4L2 devices and subdevices share a few common operations,like\n>\n> s/he/The/\n>\n> > opening and closing a device node, and perform IOCTLs on the device.\n> >\n> > With the forthcoming introduction of support for V4L2 controls, the\n> > quantity of shared code will drastically increase, as the control\n> > implementation is identical for the two.\n> >\n> > To maximize code re-use and avoid duplications, provide a V4L2Device\n> > base class which groups the common operations and members.\n> >\n> > The newly introduced base class provides methods to open/close a device\n> > node, access the file descriptor, and perform IOCTLs on the device.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Kieran Bingham \n>\n>\n> Thank you - this is much easier to review now :-D\n>\n>\n> A few comments below:\n>\n> > ---\n> > src/libcamera/include/v4l2_device.h | 36 ++++++\n> > src/libcamera/include/v4l2_subdevice.h | 6 +-\n> > src/libcamera/include/v4l2_videodevice.h | 5 +-\n> > src/libcamera/meson.build | 2 +\n> > src/libcamera/v4l2_device.cpp | 148 +++++++++++++++++++++++\n> > src/libcamera/v4l2_subdevice.cpp | 66 +++-------\n> > src/libcamera/v4l2_videodevice.cpp | 68 ++++-------\n> > 7 files changed, 227 insertions(+), 104 deletions(-)\n> > create mode 100644 src/libcamera/include/v4l2_device.h\n> > create mode 100644 src/libcamera/v4l2_device.cpp\n> >\n>\n\n[snip]\n\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int V4L2Device::ioctl(unsigned long request, void *argp)\n> > +{\n> > +\t/*\n> > +\t * Printing out an error message is usually better performed\n> > +\t * in the caller, which can provide more context.\n> > +\t */\n> > +\tif (::ioctl(fd_, request, argp) < 0)\n> > +\t\treturn -errno;\n>\n> I /really/ like this ... but note how many of the call sites for\n> V4L2Device::ioctl() still work with the -errno fuss, which should now\n> just deal directly with 'ret' where it's used.\n>\n> 'man ioctl' states:\n>\n> RETURN VALUE\n> Usually, on success zero is returned. A few ioctl() requests\n> use the return value as an output parameter and return a\n> nonnegative value on success. On error, -1 is returned, and\n> errno is set appropriately.\n>\n>\n> I really like simplifying the -1, errno check pattern that is\n> everywhere. But do we need to be sure we'll never need the return code\n> of the ioctl?\n>\n> (I don't think we do - and we /could/ have a different call for that if\n> necessary)\n>\n\nI agree, I think we're safe if we check for < 0 as it is done now.\n\n>\n>\n> > +\n> > +\treturn 0;\n> > +}\n> >\n\n[snip]\n\n> > /**\n> > @@ -462,7 +440,7 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n> > \tint ret;\n> >\n> > \tv4l2Format.type = bufferType_;\n> > -\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);\n> > +\tret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n>\n>\n> I really like that ioctl now knows about the FD internally and thus is\n> simpler :)\n>\n> \"do this action with this structure...\"\n>\n>\n> > \tif (ret) {\n> > \t\tret = -errno;\n> > \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n>\n> This is an example of things that I think should now be:\n>\n> ret = ioctl(VIDIOC_G_FMT, &v4l2Format);\n> if (ret) {\n> \tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(ret);\n> \t...\n> }\n>\n> That probably applies to all uses of V4L2Device::ioctl()\n\nYes indeed. I'll fix and resend.\n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F354A61D60\n\tfor ;\n\tWed, 12 Jun 2019 18:30:02 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 7E9A3C0009;\n\tWed, 12 Jun 2019 16:30:02 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 12 Jun 2019 18:31:13 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190612163113.ef4ltohhc7ic7kx5@uno.localdomain>","References":"<20190612133817.32731-1-jacopo@jmondi.org>\n\t<20190612133817.32731-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"affa7sx7zwpydykq\"","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: Introduce\n\tV4L2Device base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 12 Jun 2019 16:30:03 -0000"}}]