[{"id":5291,"web_url":"https://patchwork.libcamera.org/comment/5291/","msgid":"<20200619233050.GO5823@pendragon.ideasonboard.com>","date":"2020-06-19T23:30:50","subject":"Re: [libcamera-devel] [PATCH v2 01/17] v4l2: v4l2_camera_file: Add\n\tV4L2CameraFile to model the opened camera file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jun 19, 2020 at 02:41:07PM +0900, Paul Elder wrote:\n> With relation to opening files, the kernel has three objects related to\n> files:\n> - inodes, that represent files on disk\n> - file objects, that are allocated at open() time and store all data\n> related to the open file\n> - file descriptors, that are integers that map to a file\n> \n> In the V4L2 compatibility layer, V4L2CameraProxy, which wraps a single\n> libcamera camera via V4L2Camera, is more or less equivalent to the\n> inode. We also already have file descriptors (that are really eventfds)\n> that mirror the file descriptors. Here we create a V4L2CameraFile to\n> model the file objects, to contain information related to the open file,\n> namely if the file has been opened as non-blocking, and the V4L2\n> priority (to support VIDIOC_G/S_PRIORITY later on). This new class\n> allows us to more cleanly support multiple open later on, since we can\n> move out of V4L2CameraProxy the handling of mapping the fd to the open\n> file information.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> New in v2\n> ---\n> src/v4l2/meson.build | 1 +\n> src/v4l2/v4l2_camera_file.cpp | 45 +++++++++++++++++++++++++++++++++++\n> src/v4l2/v4l2_camera_file.h | 35 +++++++++++++++++++++++++++\n> 3 files changed, 81 insertions(+)\n> create mode 100644 src/v4l2/v4l2_camera_file.cpp\n> create mode 100644 src/v4l2/v4l2_camera_file.h\n> \n> diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build\n> index f2e4aaf..e3838f0 100644\n> --- a/src/v4l2/meson.build\n> +++ b/src/v4l2/meson.build\n> @@ -2,6 +2,7 @@\n> \n> v4l2_compat_sources = files([\n> 'v4l2_camera.cpp',\n> + 'v4l2_camera_file.cpp',\n> 'v4l2_camera_proxy.cpp',\n> 'v4l2_compat.cpp',\n> 'v4l2_compat_manager.cpp',\n> diff --git a/src/v4l2/v4l2_camera_file.cpp b/src/v4l2/v4l2_camera_file.cpp\n> new file mode 100644\n> index 0000000..8916729\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera_file.cpp\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n\nHappy new year :-)\n\n> + *\n> + * v4l2_camera_file.h - V4L2 compatibility camera file descriptor information\n\ns/descriptor // as this is the file, not the file descriptor ?\n\n> + */\n> +\n> +#include \"v4l2_camera_file.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +#include \"v4l2_camera_proxy.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(V4L2Compat);\n\nYou don't use LOG() here, so you can drop this, as well as the #include.\n\n> +\n> +V4L2CameraFile::V4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy)\n> +\t: priority_(V4L2_PRIORITY_DEFAULT), proxy_(proxy),\n> +\t nonBlocking_(nonBlocking), efd_(efd)\n> +{\n> +\tproxy_->open(nonBlocking);\n> +}\n> +\n> +V4L2CameraFile::~V4L2CameraFile()\n> +{\n> +\tproxy_->close();\n> +}\n> +\n> +V4L2CameraProxy *V4L2CameraFile::proxy()\n> +{\n> +\treturn proxy_;\n> +}\n> +\n> +bool V4L2CameraFile::nonBlocking()\n> +{\n> +\treturn nonBlocking_;\n> +}\n> +\n> +int V4L2CameraFile::efd()\n> +{\n> +\treturn efd_;\n> +}\n> diff --git a/src/v4l2/v4l2_camera_file.h b/src/v4l2/v4l2_camera_file.h\n> new file mode 100644\n> index 0000000..cf282e9\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera_file.h\n> @@ -0,0 +1,35 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n\n2020 here too.\n\n> + *\n> + * v4l2_camera_file.h - V4L2 compatibility camera file descriptor information\n> + */\n> +\n> +#ifndef __V4L2_CAMERA_FILE_H__\n> +#define __V4L2_CAMERA_FILE_H__\n> +\n> +#include \n> +\n> +class V4L2CameraProxy;\n> +\n> +class V4L2CameraFile\n> +{\n> +public:\n> +\tV4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy);\n> +\t~V4L2CameraFile();\n> +\n> +\tV4L2CameraProxy *proxy();\n> +\n> +\tbool nonBlocking();\n> +\tint efd();\n\nYou can make those three methods const. I would also inline them as\nthey're very simple.\n\n> +\n> +\tenum v4l2_priority priority_;\n\nThis is the only public field, it stands out a little bit. Would the\nfollowing be better ?\n\n\tenum v4l2_priority priority() const { return priority_; }\n\tvoid setPriority(enum v4l2_priority priority) { priority_ = priority; }\n\nThe compiler will optimize all that.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +private:\n> +\tV4L2CameraProxy *proxy_;\n> +\n> +\tbool nonBlocking_;\n> +\tint efd_;\n> +};\n> +\n> +#endif /* __V4L2_CAMERA_FILE_H__ */","headers":{"Return-Path":"","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 5F79760710\n\tfor ;\n\tSat, 20 Jun 2020 01:31:14 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CDCE1552;\n\tSat, 20 Jun 2020 01:31:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"J5w/4rXV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592609474;\n\tbh=pk1XZcGMY6by3axYK7R5KCRs7lGXkEqDC5BY6OaJZPw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J5w/4rXVKCScibtWtTLIBSYEsYw2iLgrJ4pjRa+Hh1FQ2seJ2XnWhkK4EU4zZ+YV3\n\t74umJyEiiVYNAWt3IMyzPS/KmLIJ6PRWcaYkVIkOlPJHM3b+OKgjHu2gZ4WhTXhdqS\n\trc1/WLAkAHu+PkrkYTsMJnkNuFqzpr6iTG12VxEY=","Date":"Sat, 20 Jun 2020 02:30:50 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200619233050.GO5823@pendragon.ideasonboard.com>","References":"<20200619054123.19052-1-paul.elder@ideasonboard.com>\n\t<20200619054123.19052-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200619054123.19052-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 01/17] v4l2: v4l2_camera_file: Add\n\tV4L2CameraFile to model the opened camera file","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 19 Jun 2020 23:31:14 -0000"}}]