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

4081 diff mbox series

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@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

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@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@ideasonboard.com>

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