[libcamera-devel] [PATCH v2 01/17] v4l2: v4l2_camera_file: Add V4L2CameraFile to model the opened camera file

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jun 19, 2020 at 02:41:07PM +0900, Paul Elder wrote:
> With relation to opening files, the kernel has three objects related to
> files:
> - inodes, that represent files on disk
> - file objects, that are allocated at open() time and store all data
>   related to the open file
> - file descriptors, that are integers that map to a file
> 
> In the V4L2 compatibility layer, V4L2CameraProxy, which wraps a single
> libcamera camera via V4L2Camera, is more or less equivalent to the
> inode. We also already have file descriptors (that are really eventfds)
> that mirror the file descriptors. Here we create a V4L2CameraFile to
> model the file objects, to contain information related to the open file,
> namely if the file has been opened as non-blocking, and the V4L2
> priority (to support VIDIOC_G/S_PRIORITY later on). This new class
> allows us to more cleanly support multiple open later on, since we can
> move out of V4L2CameraProxy the handling of mapping the fd to the open
> file information.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v2
> ---
>  src/v4l2/meson.build          |  1 +
>  src/v4l2/v4l2_camera_file.cpp | 45 +++++++++++++++++++++++++++++++++++
>  src/v4l2/v4l2_camera_file.h   | 35 +++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+)
>  create mode 100644 src/v4l2/v4l2_camera_file.cpp
>  create mode 100644 src/v4l2/v4l2_camera_file.h
> 
> diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> index f2e4aaf..e3838f0 100644
> --- a/src/v4l2/meson.build
> +++ b/src/v4l2/meson.build
> @@ -2,6 +2,7 @@
>  
>  v4l2_compat_sources = files([
>      'v4l2_camera.cpp',
> +    'v4l2_camera_file.cpp',
>      'v4l2_camera_proxy.cpp',
>      'v4l2_compat.cpp',
>      'v4l2_compat_manager.cpp',
> diff --git a/src/v4l2/v4l2_camera_file.cpp b/src/v4l2/v4l2_camera_file.cpp
> new file mode 100644
> index 0000000..8916729
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera_file.cpp
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.

Happy new year :-)

> + *
> + * v4l2_camera_file.h - V4L2 compatibility camera file descriptor information

s/descriptor // as this is the file, not the file descriptor ?

> + */
> +
> +#include "v4l2_camera_file.h"
> +
> +#include <linux/videodev2.h>
> +
> +#include "libcamera/internal/log.h"
> +
> +#include "v4l2_camera_proxy.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(V4L2Compat);

You don't use LOG() here, so you can drop this, as well as the #include.

> +
> +V4L2CameraFile::V4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy)
> +	: priority_(V4L2_PRIORITY_DEFAULT), proxy_(proxy),
> +	  nonBlocking_(nonBlocking), efd_(efd)
> +{
> +	proxy_->open(nonBlocking);
> +}
> +
> +V4L2CameraFile::~V4L2CameraFile()
> +{
> +	proxy_->close();
> +}
> +
> +V4L2CameraProxy *V4L2CameraFile::proxy()
> +{
> +	return proxy_;
> +}
> +
> +bool V4L2CameraFile::nonBlocking()
> +{
> +	return nonBlocking_;
> +}
> +
> +int V4L2CameraFile::efd()
> +{
> +	return efd_;
> +}
> diff --git a/src/v4l2/v4l2_camera_file.h b/src/v4l2/v4l2_camera_file.h
> new file mode 100644
> index 0000000..cf282e9
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera_file.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.

2020 here too.

> + *
> + * v4l2_camera_file.h - V4L2 compatibility camera file descriptor information
> + */
> +
> +#ifndef __V4L2_CAMERA_FILE_H__
> +#define __V4L2_CAMERA_FILE_H__
> +
> +#include <linux/videodev2.h>
> +
> +class V4L2CameraProxy;
> +
> +class V4L2CameraFile
> +{
> +public:
> +	V4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy);
> +	~V4L2CameraFile();
> +
> +	V4L2CameraProxy *proxy();
> +
> +	bool nonBlocking();
> +	int efd();

You can make those three methods const. I would also inline them as
they're very simple.

> +
> +	enum v4l2_priority priority_;

This is the only public field, it stands out a little bit. Would the
following be better ?

	enum v4l2_priority priority() const { return priority_; }
	void setPriority(enum v4l2_priority priority) { priority_ = priority; }

The compiler will optimize all that.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +private:
> +	V4L2CameraProxy *proxy_;
> +
> +	bool nonBlocking_;
> +	int efd_;
> +};
> +
> +#endif /* __V4L2_CAMERA_FILE_H__ */

-- 
Regards,

Laurent Pinchart