[libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device object
Jacopo Mondi
jacopo at jmondi.org
Wed Jan 16 12:11:43 CET 2019
Hi Kieran,
On Wed, Jan 16, 2019 at 10:50:13AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/01/2019 08:44, Jacopo Mondi wrote:
> > Hi Kieran,
> > a few more comments, please bear with me a little longer and thanks
> > for this series
> >
> > On Tue, Jan 15, 2019 at 11:19:08PM +0000, Kieran Bingham wrote:
> >> Provide a helper V4L2 device object capable of interacting with the
> >> V4L2 Linux Kernel APIs.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> src/libcamera/include/v4l2_device.h | 45 +++++++++
> >> src/libcamera/meson.build | 2 +
> >> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++
> >> 3 files changed, 197 insertions(+)
> >> create mode 100644 src/libcamera/include/v4l2_device.h
> >> create mode 100644 src/libcamera/v4l2_device.cpp
> >>
> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >> new file mode 100644
> >> index 000000000000..b0f5e9689654
> >> --- /dev/null
> >> +++ b/src/libcamera/include/v4l2_device.h
> >> @@ -0,0 +1,45 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * v4l2_device.h - V4L2 Device
> >> + */
> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> >> +#define __LIBCAMERA_V4L2_DEVICE_H__
> >> +
> >> +#include <string>
> >> +
> >> +#include <linux/videodev2.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +class V4L2Capability : public v4l2_capability
> >> +{
> >> +public:
> >> + std::string getDriver() const { return std::string((char *)driver); }
> >> + std::string getCard() const { return std::string((char *)card); }
> >
> > I still feel extending a kernel provided struct into an object is meh.
> > Here you could have returned references to two members, intialized
> > with the values from struct v4l2_capability....
>
> I think this is a really good way to keep methods and code which
> interacts with the struct v4l2_capability together, and treats the C
> structure like an object (which it essentially is, it has defined data
> structure and it's usage is defined by the kernel API).
>
> It doesn't extend the size of storage - and only defines how to interact
> with the data structures.
>
> The getDriver() and getCard() wraps the casting required again into the
> class definition so that it doesn't have to be done in arbitrary places
> in the code.
>
> I expect to add next:
>
> V4L2Device::driverName() { return caps_.getDriver(); }
> V4L2Device::deviceName() { return caps_.getCard(); }
> // Or ? getDeviceName()?
> V4L2Device::busName() { return caps_.getBus(); }
> // (Yes, I'll add this one in to the V4L2Capabiltiy)
>
> I anticipate that these strings will be useful to you in the pipeline
> classes.
>
> The UVC pipeline manager should certainly use them.
>
One note I missed in both my previous comments: getter methods are named as
the member field they access, without the "get" prefix.
>
> > I won't push anymore on this though.
>
> I don't mind the push ... I knew this 'wrapping' would be potentially
> controversial it's partly me trying to experiment with good ways to
> interact with the Kernel API.
>
> At the moment, I must say I really like it though ...
> <waits for the tumbleweed to pass if I'm the only one>
>
> Especially with the ease and cleanliness for exposing the capability
> strings.
>
I'll let other express concerns, if any. Otherwise that's fine with
me.
> >> +
> >> + bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
> >
> > You pointed out in your self-review this was meant to be changed,
> > right?
>
> Half-right.
>
> Then based on your other comments regarding MPLANE/Single Plane I
> decided to drop MPLANE support.
>
> We don't need it yet, the IPU3 doesn't support MPLANE, the RCAR-VIN
> doesn't support MPLANE, UVC doesn't support MPLANE ...
>
Are you sure?
- entity 4: ipu3-cio2 0 (1 pad, 1 link)
type Node subtype V4L flags 0
device node name /dev/video0
# v4l2-ctl -D -d /dev/video0
Driver Info (not using libv4l2):
Driver name : ipu3-cio2
Card type : Intel IPU3 CIO2
Bus info : PCI:0000:00:14.3
Driver version: 4.20.0
Capabilities : 0x84201000
Video Capture Multiplanar <---------------
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x04201000
Video Capture Multiplanar
Streaming
Extended Pix Format
>
> >> + bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }
> >> +};
> >> +
> >> +class V4L2Device
> >> +{
> >> +public:
> >> + V4L2Device() = delete;
> >
> > Should this be private?
>
> I wondered that - but I figured - it doesn't matter if it's public or
> private. It's declaring that it's gone. And I thought it was better to
> keep the constructors (or lack of) and destructors together.
>
> If this is something we want to standardize (keeping deletions in
> private:) I can move it.
>
No big deal, right...
>
> >
> >> + V4L2Device(const std::string &devnode);
> >> + ~V4L2Device();
> >> +
> >> + int open();
> >> + bool isOpen() const;
> >> + void close();
> >> +
> >> +private:
> >> + std::string devnode_;
> >> + int fd_;
> >> + V4L2Capability caps_;
> >> +};
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index abf9a71d4172..f9f25c0ecf15 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -11,6 +11,7 @@ libcamera_sources = files([
> >> 'pipeline_handler.cpp',
> >> 'signal.cpp',
> >> 'timer.cpp',
> >> + 'v4l2_device.cpp',
> >> ])
> >>
> >> libcamera_headers = files([
> >> @@ -21,6 +22,7 @@ libcamera_headers = files([
> >> 'include/media_object.h',
> >> 'include/pipeline_handler.h',
> >> 'include/utils.h',
> >> + 'include/v4l2_device.h',
> >> ])
> >>
> >> libcamera_internal_includes = include_directories('include')
> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >> new file mode 100644
> >> index 000000000000..8c49d3ff56e2
> >> --- /dev/null
> >> +++ b/src/libcamera/v4l2_device.cpp
> >> @@ -0,0 +1,150 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * v4l2_device.cpp - V4L2 Device
> >> + */
> >> +
> >> +#include <fcntl.h>
> >> +#include <string.h>
> >> +#include <sys/ioctl.h>
> >> +#include <sys/mman.h>
> >> +#include <unistd.h>
> >> +
> >> +#include "log.h"
> >> +#include "v4l2_device.h"
> >> +
> >> +/**
> >> + * \file v4l2_device.h
> >> + * \brief V4L2 Device API
> >> + */
> >> +namespace libcamera {
> >> +
> >> +/**
> >> + * \class V4L2Capability
> >> + * \brief struct v4l2_capability object wrapper and helpers
> >> + *
> >> + * The V4L2Capability class manages the information returned by the
> >> + * VIDIOC_QUERYCAP ioctl.
> >> + */
> >> +
> >> +/**
> >> + * \fn std::string V4L2Capability::getDriver()
> >> + * \brief Retrieve the driver module name
> >> + * \return The string containing the name of the driver module
> >> + */
> >> +
> >> +/**
> >> + * \fn std::string V4L2Capability::getCard()
> >> + * \brief Retrieve the device card name
> >> + * \return The string containing the device name
> >
> > s/card/device ?
>
> I want to respect the existing naming of the V4L2 API. It's called a
> card there.
>
> It can be exposed through the V4L2Device API as a deviceName() as
> mentioned above.
>
>
> >> + */
> >> +
> >> +/**
> >> + * \fn bool V4L2Capability::isCapture()
> >> + * \brief Identify if the device is capable of capturing video
> >> + * \return boolean true if the device provides video frames
> >> + */
> >> +
> >> +/**
> >> + * \fn bool V4L2Capability::hasStreaming()
> >> + * \brief Determine if the device can perform Streaming IO
> >> + * \return boolean true if the device provides Streaming IO IOCTLs
> >> + */
> >> +
> >> +/**
> >> + * \class V4L2Device
> >> + * \brief V4L2Device object and API.
> >> + *
> >> + * The V4L2 Device API class models an instance of a V4L2 device node.
> >> + * It is constructed with the path to a V4L2 video device node. The device node
> >> + * is only opened upon a call to open() which must be checked for success.
> >> + *
> >> + * The device capabilities are validated and the device is rejected if it is
> >> + * not a suitable capture device.
> >> + *
> >> + * No API call (except open(), isOpen() and close()) shall be called on an
> >> + * unopened device instance.
> >> + *
> >> + * Upon destruction any device left open will be closed, and any resources
> >> + * released.
> >> + */
> >> +
> >> +/**
> >> + * \brief Construct a V4L2Device
> >> + * \param devnode The file-system path to the video device node
> >> + */
> >> +V4L2Device::V4L2Device(const std::string &devnode)
> >> + : devnode_(devnode), fd_(-1)
> >> +{
> >> +}
> >> +
> >> +V4L2Device::~V4L2Device()
> >> +{
> >> + close();
> >> +}
> >> +
> >> +/**
> >> + * \brief Open a V4L2 device and query properties from the device.
> >> + * \return 0 on success, or a negative error code otherwise
> >> + */
> >> +int V4L2Device::open()
> >> +{
> >> + int ret;
> >> +
> >> + if (isOpen()) {
> >> + LOG(Error) << "Device already open";
> >> + return -EBUSY;
> >> + }
> >> +
> >> + ret = ::open(devnode_.c_str(), O_RDWR);
> >> + if (ret < 0) {
> >> + ret = -errno;
> >> + LOG(Error) << "Failed to open V4L2 device " << devnode_
> >> + << " : " << strerror(ret);
> >
> > strerror(-ret)
> >
> > Even if in this case you could have used errno directly. Sorry, my
> > comment might has mis-lead you
>
> Ahh oops - and good spot.
>
> But yes - I'd much rather reference the errno directly for strerror().
>
>
> >
> >> + return ret;
> >> + }
> >> + fd_ = ret;
> >> +
> >> + ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> >
> > As you pointed out, this might be the compiler performing down-casting
> > to a plain 'struct v4l2_capability'
>
> Yes, which is the desired result.
>
> >
> >> + if (ret < 0) {
> >> + LOG(Error) << "Failed to query device capabilities: " << strerror(errno);
> >> + return -errno;
> >> + }
> >> +
> >> + LOG(Debug) << "Opened '" << devnode_ << "' " << caps_.getDriver() << ": " << caps_.getCard();
> >> +
> >> + if (!(caps_.isCapture())) {
> >
> > if (!caps_.isCapture())
>
> Yes :)
>
> >
> >> + LOG(Error) << "Device is not a capture device";
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (!(caps_.hasStreaming())) {
>
> And here...
>
> >> + LOG(Error) << "Device does not support streaming IO";
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief Check if device is successfully opened
> >> + */
> >> +bool V4L2Device::isOpen() const
> >> +{
> >> + return (fd_ != -1);
> >
> > Ah, I see what you've done here (return with no () )
> >
> > bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
>
> Ah I've been caught! (ok well it wasn't intentional hehe)
>
> I /almost/ want to add () to the isCapture() but I don't want to
> increase that line length...
>
> /me cries a little and does:
>
> s/(fd_ != -1)/fd != -1/
>
As you like the most!
Thanks
j
> >
> >> +}
> >> +
> >> +/**
> >> + * \brief Close the device, releasing any resources acquired by open()
> >> + */
> >> +void V4L2Device::close()
> >> +{
> >> + if (fd_ < 0)
> >> + return;
> >> +
> >> + ::close(fd_);
> >> + fd_ = -1;
> >> +}
> >> +
> >> +} /* namespace libcamera */
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190116/bf0a3b26/attachment.sig>
More information about the libcamera-devel
mailing list