[{"id":1909,"web_url":"https://patchwork.libcamera.org/comment/1909/","msgid":"<20190618210745.GG23556@pendragon.ideasonboard.com>","date":"2019-06-18T21:07:45","subject":"Re: [libcamera-devel] [PATCH v4 2/2] libcamera: Introduce\n\tV4L2Device base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Jun 13, 2019 at 09:49:55AM +0200, Jacopo Mondi wrote:\n> The V4L2 devices and subdevices share a few common operations,like\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\nDrastically may be a bit of an overstatement ;-)\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 <jacopo@jmondi.org>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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            | 131 +++++++++++++++++++++++\n>  src/libcamera/v4l2_subdevice.cpp         |  69 +++---------\n>  src/libcamera/v4l2_videodevice.cpp       |  84 +++++----------\n>  7 files changed, 212 insertions(+), 121 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..a73e1b600500\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 <string>\n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device\n> +{\n> +public:\n> +\tvirtual ~V4L2Device() { close(); }\n\nI don't think the constructor needs to be virtual, as there are no other\nvirtual methods in the class, and we're not going to delete the\nV4L2Device instance directly (I would make the destructor protected to\nensure that).\n\n> +\n> +protected:\n> +\tV4L2Device();\n> +\n> +\tint fd() { return fd_; }\n> +\n> +\tint open(const std::string &pathname, unsigned int flags);\n> +\tvoid close();\n> +\tbool isOpen() const;\n\nShouldn't isOpen() and close() be public ? Otherwise a user of\nV4L2VideoDevice or V4L2Subdevice won't be able to call them.\n\nI think you can inline isOpen() too.\n\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 <libcamera/signal.h>\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..6cbfc212a725\n> --- /dev/null\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -0,0 +1,131 @@\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 video devices and subdevices\n> + */\n> +\n> +#include \"v4l2_device.h\"\n> +\n> +#include <fcntl.h>\n> +#include <string.h>\n> +#include <sys/ioctl.h>\n> +#include <unistd.h>\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\nWould it make sense to pass the device node name to the constructor and\nremove it from the open() method ?\n\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\nI think I would keep this message in the callers, otherwise it won't\nbenefit from the inheritance from Loggable. Another option would be to\nmake V4L2Device inherit from loggable and move the logPrefix() method\nthere, while still keeping V4L2VideoDevice::logPrefix() to also print\nthe direction. I think that would be my favourite option.\n\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> +void V4L2Device::close()\n> +{\n> +\tif (fd_ < 0)\n> +\t\treturn;\n> +\n> +\tif (::close(fd_) < 0)\n> +\t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> +\t\t\t\t << strerror(errno);\n> +\tfd_ = -1;\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 pointer to the IOCTL argument\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> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 6681c1920065..52244d2ac3e1 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\nThis should be replaced with\n\n\tif (ret < 0 && ret != -EINVAL && ret != -ENOTTY)\n\n>  \t\tret = -errno;\n\nand this line should be removed.\n\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,9 @@ 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 +247,9 @@ 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 +299,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 +311,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,\n>  \n>  \tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n>  \t\tret = -errno;\n\nSame here.\n\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 +337,9 @@ 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..2172cf25bfb6 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,23 +305,12 @@ 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>  \t\t\t<< \"Failed to query device capabilities: \"\n>  \t\t\t<< strerror(-ret);\n> @@ -342,20 +331,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 +357,18 @@ 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\nHow about\n\n\tif (isOpen())\n\n>  \t\treturn;\n>  \n> +\tclose();\n\nWon't the compiler call V4L2VideoDevice::close() ? Shouldn't you write\n\n\tV4L2Device::close();\n\n> +\n>  \treleaseBuffers();\n\nThis calls an ioctl, so the close() call should be moved to the end of\nthe function.\n\n>  \tdelete fdEvent_;\n> -\n> -\t::close(fd_);\n> -\tfd_ = -1;\n>  }\n>  \n>  /**\n> @@ -462,9 +441,8 @@ 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>  \tif (ret) {\n> -\t\tret = -errno;\n>  \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n>  \t\treturn ret;\n>  \t}\n> @@ -488,9 +466,8 @@ 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>  \t\treturn ret;\n>  \t}\n> @@ -516,9 +493,8 @@ 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>  \t\treturn ret;\n>  \t}\n> @@ -554,9 +530,8 @@ 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>  \t\treturn ret;\n>  \t}\n> @@ -584,9 +559,8 @@ 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>  \t\treturn ret;\n>  \t}\n> @@ -613,9 +587,8 @@ 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>  \t\treturn ret;\n>  \t}\n> @@ -643,9 +616,8 @@ 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>  \t\t\t<< \"Unable to request \" << count << \" buffers: \"\n>  \t\t\t<< strerror(-ret);\n> @@ -695,9 +667,8 @@ 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>  \t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n>  \t\t\t\t<< strerror(-ret);\n> @@ -748,9 +719,8 @@ 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>  \t\t\t<< \"Failed to export buffer: \" << strerror(-ret);\n>  \t\treturn ret;\n> @@ -855,9 +825,8 @@ 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>  \t\t\t<< \"Failed to queue buffer \" << buf.index << \": \"\n>  \t\t\t<< strerror(-ret);\n> @@ -892,9 +861,8 @@ 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>  \t\t\t<< \"Failed to dequeue buffer: \" << strerror(-ret);\n>  \t\treturn nullptr;\n> @@ -952,9 +920,8 @@ 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>  \t\t\t<< \"Failed to start streaming: \" << strerror(-ret);\n>  \t\treturn ret;\n> @@ -975,9 +942,8 @@ 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>  \t\t\t<< \"Failed to stop streaming: \" << strerror(-ret);\n>  \t\treturn ret;","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 3AC3861D59\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 18 Jun 2019 23:08:03 +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 9EDC8D5;\n\tTue, 18 Jun 2019 23:08:02 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560892082;\n\tbh=5OHoi3TASfPc/ltZz5xkQWtOmZp8clFXjf1Vp2aZ0Vs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ReWPLA2iHnmo3cPTdPQVbezKnTKUG/tt7YY6K7J0afLI7TYO6P5uTcvVHQKtuyGrk\n\tUQvhwukZBCFm/ZCuoYMtqAsGOxwVg8xwYyq76FZA9fuOih5X8SNV3SBD94QynU7MB1\n\tkcvc7t+Mf8phXYYBhDZ86Ur6Cx+pgcWJMSnpsWmM=","Date":"Wed, 19 Jun 2019 00:07:45 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190618210745.GG23556@pendragon.ideasonboard.com>","References":"<20190613074955.14512-1-jacopo@jmondi.org>\n\t<20190613074955.14512-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190613074955.14512-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 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":"<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":"Tue, 18 Jun 2019 21:08:03 -0000"}},{"id":1927,"web_url":"https://patchwork.libcamera.org/comment/1927/","msgid":"<20190619073423.54fc2cxcq5jmkg2t@uno.localdomain>","date":"2019-06-19T07:34:23","subject":"Re: [libcamera-devel] [PATCH v4 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 Laurent,\n\nOn Wed, Jun 19, 2019 at 12:07:45AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 13, 2019 at 09:49:55AM +0200, Jacopo Mondi wrote:\n> > The V4L2 devices and subdevices share a few common operations,like\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> Drastically may be a bit of an overstatement ;-)\n>\n\nWell, considering how thin it is now... :)\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 <jacopo@jmondi.org>\n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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            | 131 +++++++++++++++++++++++\n> >  src/libcamera/v4l2_subdevice.cpp         |  69 +++---------\n> >  src/libcamera/v4l2_videodevice.cpp       |  84 +++++----------\n> >  7 files changed, 212 insertions(+), 121 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..a73e1b600500\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 <string>\n> > +\n> > +namespace libcamera {\n> > +\n> > +class V4L2Device\n> > +{\n> > +public:\n> > +\tvirtual ~V4L2Device() { close(); }\n>\n> I don't think the constructor needs to be virtual, as there are no other\n> virtual methods in the class, and we're not going to delete the\n> V4L2Device instance directly (I would make the destructor protected to\n> ensure that).\n>\n\nIndeed, V4L2Device instances cannot be created directly, so no need\nfor a virtual destructor.\n\n> > +\n> > +protected:\n> > +\tV4L2Device();\n> > +\n> > +\tint fd() { return fd_; }\n> > +\n> > +\tint open(const std::string &pathname, unsigned int flags);\n> > +\tvoid close();\n> > +\tbool isOpen() const;\n>\n> Shouldn't isOpen() and close() be public ? Otherwise a user of\n> V4L2VideoDevice or V4L2Subdevice won't be able to call them.\n\nNo one calls them at the moment, appartently..\n\n>\n> I think you can inline isOpen() too.\n>\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 <libcamera/signal.h>\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..6cbfc212a725\n> > --- /dev/null\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -0,0 +1,131 @@\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 video devices and subdevices\n> > + */\n> > +\n> > +#include \"v4l2_device.h\"\n> > +\n> > +#include <fcntl.h>\n> > +#include <string.h>\n> > +#include <sys/ioctl.h>\n> > +#include <unistd.h>\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>\n> Would it make sense to pass the device node name to the constructor and\n> remove it from the open() method ?\n>\n\nWe then would have to store it, and access it at open time.\nCurrently we're not storing it, not sure it is worth doing it,\nconsidering all the users would then have to be changed.\n\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>\n> I think I would keep this message in the callers, otherwise it won't\n> benefit from the inheritance from Loggable. Another option would be to\n> make V4L2Device inherit from loggable and move the logPrefix() method\n> there, while still keeping V4L2VideoDevice::logPrefix() to also print\n> the direction. I think that would be my favourite option.\n>\n\nThis maybe then call for storing the device node path\n\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> > +void V4L2Device::close()\n> > +{\n> > +\tif (fd_ < 0)\n> > +\t\treturn;\n> > +\n> > +\tif (::close(fd_) < 0)\n> > +\t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > +\t\t\t\t << strerror(errno);\n> > +\tfd_ = -1;\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 pointer to the IOCTL argument\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> > +\treturn 0;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 6681c1920065..52244d2ac3e1 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>\n> This should be replaced with\n>\n> \tif (ret < 0 && ret != -EINVAL && ret != -ENOTTY)\n>\n> >  \t\tret = -errno;\n>\n> and this line should be removed.\n>\n\nCorrect, thanks!\n\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,9 @@ 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 +247,9 @@ 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 +299,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 +311,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,\n> >\n> >  \tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> >  \t\tret = -errno;\n>\n> Same here.\n>\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 +337,9 @@ 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..2172cf25bfb6 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,23 +305,12 @@ 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> >  \t\t\t<< \"Failed to query device capabilities: \"\n> >  \t\t\t<< strerror(-ret);\n> > @@ -342,20 +331,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 +357,18 @@ 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>\n> How about\n>\n> \tif (isOpen())\n>\n> >  \t\treturn;\n> >\n> > +\tclose();\n>\n> Won't the compiler call V4L2VideoDevice::close() ? Shouldn't you write\n>\n> \tV4L2Device::close();\n\nThis is the V4L2VideoDevice::close() method :)\n\n>\n> > +\n> >  \treleaseBuffers();\n>\n> This calls an ioctl, so the close() call should be moved to the end of\n> the function.\n>\n\nOuch, correct, I'll change it\n\nThanks\n   j\n\n> >  \tdelete fdEvent_;\n> > -\n> > -\t::close(fd_);\n> > -\tfd_ = -1;\n> >  }\n> >\n> >  /**\n> > @@ -462,9 +441,8 @@ 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> >  \tif (ret) {\n> > -\t\tret = -errno;\n> >  \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n> >  \t\treturn ret;\n> >  \t}\n> > @@ -488,9 +466,8 @@ 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> >  \t\treturn ret;\n> >  \t}\n> > @@ -516,9 +493,8 @@ 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> >  \t\treturn ret;\n> >  \t}\n> > @@ -554,9 +530,8 @@ 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> >  \t\treturn ret;\n> >  \t}\n> > @@ -584,9 +559,8 @@ 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> >  \t\treturn ret;\n> >  \t}\n> > @@ -613,9 +587,8 @@ 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> >  \t\treturn ret;\n> >  \t}\n> > @@ -643,9 +616,8 @@ 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> >  \t\t\t<< \"Unable to request \" << count << \" buffers: \"\n> >  \t\t\t<< strerror(-ret);\n> > @@ -695,9 +667,8 @@ 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> >  \t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> >  \t\t\t\t<< strerror(-ret);\n> > @@ -748,9 +719,8 @@ 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> >  \t\t\t<< \"Failed to export buffer: \" << strerror(-ret);\n> >  \t\treturn ret;\n> > @@ -855,9 +825,8 @@ 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> >  \t\t\t<< \"Failed to queue buffer \" << buf.index << \": \"\n> >  \t\t\t<< strerror(-ret);\n> > @@ -892,9 +861,8 @@ 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> >  \t\t\t<< \"Failed to dequeue buffer: \" << strerror(-ret);\n> >  \t\treturn nullptr;\n> > @@ -952,9 +920,8 @@ 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> >  \t\t\t<< \"Failed to start streaming: \" << strerror(-ret);\n> >  \t\treturn ret;\n> > @@ -975,9 +942,8 @@ 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> >  \t\t\t<< \"Failed to stop streaming: \" << strerror(-ret);\n> >  \t\treturn ret;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB01061A15\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 19 Jun 2019 09:33:15 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 4EF9640008;\n\tWed, 19 Jun 2019 07:33:09 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 19 Jun 2019 09:34:23 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190619073423.54fc2cxcq5jmkg2t@uno.localdomain>","References":"<20190613074955.14512-1-jacopo@jmondi.org>\n\t<20190613074955.14512-3-jacopo@jmondi.org>\n\t<20190618210745.GG23556@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"tqgiwb2aimzj34g3\"","Content-Disposition":"inline","In-Reply-To":"<20190618210745.GG23556@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 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":"<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":"Wed, 19 Jun 2019 07:33:16 -0000"}},{"id":1928,"web_url":"https://patchwork.libcamera.org/comment/1928/","msgid":"<20190619092404.GA5045@pendragon.ideasonboard.com>","date":"2019-06-19T09:24:04","subject":"Re: [libcamera-devel] [PATCH v4 2/2] libcamera: Introduce\n\tV4L2Device base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jun 19, 2019 at 09:34:23AM +0200, Jacopo Mondi wrote:\n> On Wed, Jun 19, 2019 at 12:07:45AM +0300, Laurent Pinchart wrote:\n> > On Thu, Jun 13, 2019 at 09:49:55AM +0200, Jacopo Mondi wrote:\n> > > The V4L2 devices and subdevices share a few common operations,like\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> > Drastically may be a bit of an overstatement ;-)\n> \n> Well, considering how thin it is now... :)\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 <jacopo@jmondi.org>\n> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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            | 131 +++++++++++++++++++++++\n> > >  src/libcamera/v4l2_subdevice.cpp         |  69 +++---------\n> > >  src/libcamera/v4l2_videodevice.cpp       |  84 +++++----------\n> > >  7 files changed, 212 insertions(+), 121 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..a73e1b600500\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 <string>\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class V4L2Device\n> > > +{\n> > > +public:\n> > > +\tvirtual ~V4L2Device() { close(); }\n> >\n> > I don't think the constructor needs to be virtual, as there are no other\n> > virtual methods in the class, and we're not going to delete the\n> > V4L2Device instance directly (I would make the destructor protected to\n> > ensure that).\n> \n> Indeed, V4L2Device instances cannot be created directly, so no need\n> for a virtual destructor.\n\nIt's not about whether they can be created directly or not.\n\nclass Base\n{\npublic:\n\t~Base() { }\n}\n\nclass Derived : public Base\n{\npublic:\n\tDerived() { resource = new char[100]; }\n\t~Derived() { delete[] resource; }\nprivate:\n\tchar *resource;\n}\n\nint main\n{\n\tDerived *d = new Derived();\n\tBase *b = d;\n\n\tdelete b;\n\treturn 0;\n}\n\nThis will leak the resource, as deleting b will call Base::~Base() only.\nIf ~Base() was virtual, deleting b would call Derived::~Derived(),\ncorrectly freeing resource.\n\nIn this case the destructor doesn't need to be virtual because no\ninstance of V4L2VideoDevice or V4L2Subdevice is deleted using a pointer\nto V4L2Device.\n\n> > > +\n> > > +protected:\n> > > +\tV4L2Device();\n> > > +\n> > > +\tint fd() { return fd_; }\n> > > +\n> > > +\tint open(const std::string &pathname, unsigned int flags);\n> > > +\tvoid close();\n> > > +\tbool isOpen() const;\n> >\n> > Shouldn't isOpen() and close() be public ? Otherwise a user of\n> > V4L2VideoDevice or V4L2Subdevice won't be able to call them.\n> \n> No one calls them at the moment, appartently..\n\nI thought so, otherwise the compiler would have told you. I think they\nshould still be made public.\n\n> > I think you can inline isOpen() too.\n> >\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 <libcamera/signal.h>\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..6cbfc212a725\n> > > --- /dev/null\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -0,0 +1,131 @@\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 video devices and subdevices\n> > > + */\n> > > +\n> > > +#include \"v4l2_device.h\"\n> > > +\n> > > +#include <fcntl.h>\n> > > +#include <string.h>\n> > > +#include <sys/ioctl.h>\n> > > +#include <unistd.h>\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> >\n> > Would it make sense to pass the device node name to the constructor and\n> > remove it from the open() method ?\n> \n> We then would have to store it, and access it at open time.\n> Currently we're not storing it, not sure it is worth doing it,\n> considering all the users would then have to be changed.\n> \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> >\n> > I think I would keep this message in the callers, otherwise it won't\n> > benefit from the inheritance from Loggable. Another option would be to\n> > make V4L2Device inherit from loggable and move the logPrefix() method\n> > there, while still keeping V4L2VideoDevice::logPrefix() to also print\n> > the direction. I think that would be my favourite option.\n> \n> This maybe then call for storing the device node path\n\nYes it would. I think that would simplify the code.\n\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> > > +void V4L2Device::close()\n> > > +{\n> > > +\tif (fd_ < 0)\n> > > +\t\treturn;\n> > > +\n> > > +\tif (::close(fd_) < 0)\n> > > +\t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > > +\t\t\t\t << strerror(errno);\n> > > +\tfd_ = -1;\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 pointer to the IOCTL argument\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> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 6681c1920065..52244d2ac3e1 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> >\n> > This should be replaced with\n> >\n> > \tif (ret < 0 && ret != -EINVAL && ret != -ENOTTY)\n> >\n> > >  \t\tret = -errno;\n> >\n> > and this line should be removed.\n> \n> Correct, thanks!\n> \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,9 @@ 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 +247,9 @@ 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 +299,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 +311,7 @@ int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,\n> > >\n> > >  \tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> > >  \t\tret = -errno;\n> >\n> > Same here.\n> >\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 +337,9 @@ 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..2172cf25bfb6 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,23 +305,12 @@ 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> > >  \t\t\t<< \"Failed to query device capabilities: \"\n> > >  \t\t\t<< strerror(-ret);\n> > > @@ -342,20 +331,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 +357,18 @@ 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> >\n> > How about\n> >\n> > \tif (isOpen())\n> >\n> > >  \t\treturn;\n> > >\n> > > +\tclose();\n> >\n> > Won't the compiler call V4L2VideoDevice::close() ? Shouldn't you write\n> >\n> > \tV4L2Device::close();\n> \n> This is the V4L2VideoDevice::close() method :)\n\nWhich then calls itself back, leading to infinite recursion :-) I wonder\nwhy this hasn't been caught by tests, it would be worth checking if\nV4L2Device::close() gets called (just log a message there, it should\nthen be obvious).\n\nAnd now that I wrote this, the reason is quite clear. close() is called\non an instance of V4L2Device by V4L2Device::V4L2Device(), so it calls\nV4L2Device::close(), not V4L2VideoDevice::close(). You need to make the\nclose method virtual in V4L2Device.\n\n> > > +\n> > >  \treleaseBuffers();\n> >\n> > This calls an ioctl, so the close() call should be moved to the end of\n> > the function.\n> \n> Ouch, correct, I'll change it\n> \n> > >  \tdelete fdEvent_;\n> > > -\n> > > -\t::close(fd_);\n> > > -\tfd_ = -1;\n> > >  }\n> > >\n> > >  /**\n> > > @@ -462,9 +441,8 @@ 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> > >  \tif (ret) {\n> > > -\t\tret = -errno;\n> > >  \t\tLOG(V4L2, Error) << \"Unable to get format: \" << strerror(-ret);\n> > >  \t\treturn ret;\n> > >  \t}\n> > > @@ -488,9 +466,8 @@ 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> > >  \t\treturn ret;\n> > >  \t}\n> > > @@ -516,9 +493,8 @@ 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> > >  \t\treturn ret;\n> > >  \t}\n> > > @@ -554,9 +530,8 @@ 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> > >  \t\treturn ret;\n> > >  \t}\n> > > @@ -584,9 +559,8 @@ 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> > >  \t\treturn ret;\n> > >  \t}\n> > > @@ -613,9 +587,8 @@ 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> > >  \t\treturn ret;\n> > >  \t}\n> > > @@ -643,9 +616,8 @@ 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> > >  \t\t\t<< \"Unable to request \" << count << \" buffers: \"\n> > >  \t\t\t<< strerror(-ret);\n> > > @@ -695,9 +667,8 @@ 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> > >  \t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> > >  \t\t\t\t<< strerror(-ret);\n> > > @@ -748,9 +719,8 @@ 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> > >  \t\t\t<< \"Failed to export buffer: \" << strerror(-ret);\n> > >  \t\treturn ret;\n> > > @@ -855,9 +825,8 @@ 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> > >  \t\t\t<< \"Failed to queue buffer \" << buf.index << \": \"\n> > >  \t\t\t<< strerror(-ret);\n> > > @@ -892,9 +861,8 @@ 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> > >  \t\t\t<< \"Failed to dequeue buffer: \" << strerror(-ret);\n> > >  \t\treturn nullptr;\n> > > @@ -952,9 +920,8 @@ 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> > >  \t\t\t<< \"Failed to start streaming: \" << strerror(-ret);\n> > >  \t\treturn ret;\n> > > @@ -975,9 +942,8 @@ 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> > >  \t\t\t<< \"Failed to stop streaming: \" << strerror(-ret);\n> > >  \t\treturn ret;","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 9619161A13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 19 Jun 2019 11:24:22 +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 E9B32333;\n\tWed, 19 Jun 2019 11:24:21 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560936262;\n\tbh=w8cLXEafdYuzA2YyHpAazhNsN7cRlYkF6g32/5HR5fs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Nmt2A5IHDtTfUb1rK09srbOuSdyyui5zFVDrgyTNjqf60oC18ygAuINVjywbYQ9uz\n\t7b2pROgHBOVAGMeWK7HH6apvn7cSZkJbBFpvplUEskt5n2iYcUlWrRiz/Y8Oc4t6oS\n\tYy1MbxU1g0CzAkTneQO8EYyPpzDgD2RyICX1gLyA=","Date":"Wed, 19 Jun 2019 12:24:04 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190619092404.GA5045@pendragon.ideasonboard.com>","References":"<20190613074955.14512-1-jacopo@jmondi.org>\n\t<20190613074955.14512-3-jacopo@jmondi.org>\n\t<20190618210745.GG23556@pendragon.ideasonboard.com>\n\t<20190619073423.54fc2cxcq5jmkg2t@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190619073423.54fc2cxcq5jmkg2t@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 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":"<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":"Wed, 19 Jun 2019 09:24:22 -0000"}}]