[libcamera-devel,3/6] libcamera: v4l2_videodevice: Support M2M devices

1752 diff mbox series

V4L2 M2M devices represent a V4L2Device with two queues. One output, and
one capture on a single device node.

Represent this by instantiating a V4L2VideoDevice for each queue type,
and preparing each device for its queue.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/v4l2_videodevice.h |  26 ++-
 src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---
 2 files changed, 201 insertions(+), 25 deletions(-)

Hi Kieran,

Thank you for the patch.

On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:
> V4L2 M2M devices represent a V4L2Device with two queues. One output, and

s/queues. One/queues: one/ (or "queues, one")

> one capture on a single device node.
> 
> Represent this by instantiating a V4L2VideoDevice for each queue type,
> and preparing each device for its queue.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_videodevice.h |  26 ++-
>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---
>  2 files changed, 201 insertions(+), 25 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index f5c8da93fcb5..24c58d787fde 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {
>  					V4L2_CAP_VIDEO_OUTPUT |
>  					V4L2_CAP_VIDEO_OUTPUT_MPLANE);
>  	}
> +	bool isM2M() const
> +	{
> +		return device_caps() & (V4L2_CAP_VIDEO_M2M |
> +					V4L2_CAP_VIDEO_M2M_MPLANE);
> +	}
>  	bool isMeta() const
>  	{
>  		return device_caps() & (V4L2_CAP_META_CAPTURE |
> @@ -123,7 +128,7 @@ public:
>  
>  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
>  
> -	int open();
> +	int open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);
>  	void close();
>  
>  	const char *driverName() const { return caps_.driver(); }
> @@ -152,6 +157,9 @@ protected:
>  	std::string logPrefix() const;
>  
>  private:
> +	int queryBufferType();
> +	int queryBufferType(enum v4l2_buf_type type);
> +
>  	int getFormatMeta(V4L2DeviceFormat *format);
>  	int setFormatMeta(V4L2DeviceFormat *format);
>  
> @@ -182,6 +190,22 @@ private:
>  	EventNotifier *fdEvent_;
>  };
>  
> +class V4L2M2MDevice
> +{
> +public:
> +	V4L2M2MDevice(const std::string &deviceNode);
> +	~V4L2M2MDevice();
> +
> +	V4L2VideoDevice *output() { return output_; }
> +	V4L2VideoDevice *capture() { return capture_; }
> +	unsigned int status() { return status_; }

The status is an unsigned int, and stores an error value that can be
negative, so there's a problem.

Furthermore, we have no status methods for the other V4L2-related
classes, so I think the open() should be split out from the constructor
to make the API consistent.

> +
> +private:
> +	V4L2VideoDevice *output_;
> +	V4L2VideoDevice *capture_;
> +	unsigned int status_;
> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 81098dd70190..4bd33af5f3de 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)
>   * \return True if the video device can capture or output images
>   */
>  
> +/**
> + * \fn V4L2Capability::isM2M()
> + * \brief Identify if the device is an M2M device
> + * \return True if the device can capture and output images using the M2M API
> + */
> +
>  /**
>   * \fn V4L2Capability::isMeta()
>   * \brief Identify if the video device captures or outputs image meta-data
> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()
>  	close();
>  }
>  
> +int V4L2VideoDevice::queryBufferType()
> +{
> +	if (caps_.isVideoCapture()) {
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> +		bufferType_ = caps_.isMultiplanar()
> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	} else if (caps_.isVideoOutput()) {
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> +		bufferType_ = caps_.isMultiplanar()
> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +	} else if (caps_.isMetaCapture()) {
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> +		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> +	} else if (caps_.isMetaOutput()) {
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> +		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> +	} else {
> +		LOG(V4L2, Error) << "Device is not a supported type";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)
> +{
> +	switch (type) {
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> +		bufferType_ = caps_.isMultiplanar()
> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +		break;
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> +		bufferType_ = caps_.isMultiplanar()
> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +		break;
> +	default:
> +		LOG(V4L2, Error) << "Unsupported buffer type";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

These methods don't rely query the buffer type, they should be renamed.
The two overloaded version are also a bit confusing :-(

> +
>  /**
>   * \brief Open a V4L2 video device and query its capabilities
> + *
> + * \param[in] fd The file descriptor to set (optional)
> + * \param[in] type The device type to operate on (optional)
> + *
> + * Opens a device or sets the file descriptor if provided, and then queries the
> + * capabilities of the device and establishes the buffer types and device events
> + * accordingly.
> + *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2VideoDevice::open()
> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)

There's very little code shared between the two implementations of
open(), I think I would make it two separate methods (both with the same
name) and remove the default parameter values in the method declaration.
The code would be easier to read, and you could keep the
queryBufferType() code inlined.

>  {
>  	int ret;
>  
> -	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> -	if (ret < 0)
> -		return ret;
> +	if (fd != -1) {
> +		ret = V4L2Device::setFd(fd);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
>  	if (ret < 0) {
> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()
>  	 * devices (POLLIN), and write notifications for OUTPUT video devices
>  	 * (POLLOUT).
>  	 */
> -	if (caps_.isVideoCapture()) {
> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> -		bufferType_ = caps_.isMultiplanar()
> -			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> -			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> -	} else if (caps_.isVideoOutput()) {
> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> -		bufferType_ = caps_.isMultiplanar()
> -			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> -			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> -	} else if (caps_.isMetaCapture()) {
> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> -		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> -	} else if (caps_.isMetaOutput()) {
> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> -		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> -	} else {
> -		LOG(V4L2, Error) << "Device is not a supported type";
> -		return -EINVAL;
> -	}
> +	if (type != V4L2_BUF_TYPE_PRIVATE)

This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE
as a magic default value for the non-M2M case is quite a bit of a hack.

> +		queryBufferType(type);
> +	else
> +		queryBufferType();
>  
>  	fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
>  	fdEvent_->setEnabled(false);
> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>  	return new V4L2VideoDevice(mediaEntity);
>  }
>  
> +/**
> + * \class V4L2M2MDevice
> + * \brief Memory2Memory device container
> + *
> + * The V4L2M2MDevice manages two V4L2Device instances on the same

s/V4L2Device/V4L2VideoDevice/

> + * deviceNode which operate together using two queues to implement the V4L2

s/deviceNode/device node/

> + * Memory to Memory API.
> + *
> + * V4L2M2MDevices are opened at the point they are created, and will return

s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the
plural, otherwise doxygen won't generate links properly)

s/will //

> + * both a capture and an output device or neither in the event of failure.
> + *
> + * The two devices should be configured and started as normal V4L2Device

s/should/shall/

> + * instances. Closing either of the devices will require recreating a new
> + * V4L2M2MDevice.

With the proposal to add an open() method, I would add a close() method
to V4L2M2MDevice that closes both V4L2VideoDevice, and document here
that the managed V4L2VideoDevice instances shall not be closed manually.

> + *
> + * The two V4L2Device instances share a single device node which is opened at

s/V4L2Device/V4L2VideoDevice/

> + * the point the V4L2M2MDevice is created.
> + */
> +
> +/**
> + * \fn V4L2M2MDevice::output

Missing \brief, and below too.

> + * \return The output V4L2Device instance
> + */
> +
> +/**
> + * \fn V4L2M2MDevice::capture
> + * \return The capture V4L2Device instance
> + */
> +
> +/**
> + * \fn V4L2M2MDevice::status
> + * \return The construction status of the V4L2M2MDevice

You should document what the status value contains. Or rather drop this
method as it won't be needed with open(). Otherwise I'd recommend
replacing status() with an isValid() method that returns a bool, that
would make the API more explicit.

> + */
> +
> +/**
> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
> + *
> + * The deviceNode will be opened and shared across two V4L2VideoDevice

"will" ? It's done in this method, not later :-)

> + * instances. One to control the output stream, and one to control the capture
> + * stream.

The second sentence has no verb.

> + *
> + * After construction, the status() must be checked to validate the instance.
> + */
> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)
> +	: status_(0)
> +{
> +	int fd[2] = { -1, -1 };
> +	int ret;
> +
> +	output_ = new V4L2VideoDevice(deviceNode);
> +	capture_ = new V4L2VideoDevice(deviceNode);
> +
> +	/* Both V4L2Devices will have the same deviceNode. */

More than the same device node, they use the same file handle.

	/*
	 * Both the output and capture V4L2Device instances use the same
	 * file handle for the same device node.
	 */

> +	fd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
> +	if (fd[0] < 0) {
> +		ret = -errno;
> +		LOG(V4L2, Error)
> +			<< "Failed to open V4L2 M2M device: " << strerror(-ret);
> +		goto err;
> +	}
> +
> +	fd[1] = dup(fd[0]);
> +	if (fd[1] < 0) {
> +		ret = -errno;
> +		LOG(V4L2, Error)
> +			<< "Failed to duplicate M2M device: " << strerror(-ret);
> +
> +		goto err;
> +	}
> +
> +	ret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
> +	if (ret)
> +		goto err;
> +
> +	ret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +	if (ret)
> +		goto err;
> +
> +	return;
> +
> +err:
> +	delete capture_;
> +	delete output_;
> +
> +	capture_ = nullptr;
> +	output_ = nullptr;
> +
> +	status_ = ret;
> +
> +	close(fd[1]);
> +	close(fd[0]);

This is racy. delete output_ above will close fd[0] if the
output_->open() call succeeded, and another thread could open a file
that reuses the same file descriptor before the close() call here.

> +}
> +
> +V4L2M2MDevice::~V4L2M2MDevice()
> +{
> +	delete capture_;
> +	delete output_;
> +}
> +
>  } /* namespace libcamera */

Hi Laurent,

On 08/08/2019 22:03, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:
>> V4L2 M2M devices represent a V4L2Device with two queues. One output, and
> 
> s/queues. One/queues: one/ (or "queues, one")
> 
>> one capture on a single device node.
>>
>> Represent this by instantiating a V4L2VideoDevice for each queue type,
>> and preparing each device for its queue.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/libcamera/include/v4l2_videodevice.h |  26 ++-
>>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---
>>  2 files changed, 201 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
>> index f5c8da93fcb5..24c58d787fde 100644
>> --- a/src/libcamera/include/v4l2_videodevice.h
>> +++ b/src/libcamera/include/v4l2_videodevice.h
>> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {
>>  					V4L2_CAP_VIDEO_OUTPUT |
>>  					V4L2_CAP_VIDEO_OUTPUT_MPLANE);
>>  	}
>> +	bool isM2M() const
>> +	{
>> +		return device_caps() & (V4L2_CAP_VIDEO_M2M |
>> +					V4L2_CAP_VIDEO_M2M_MPLANE);
>> +	}
>>  	bool isMeta() const
>>  	{
>>  		return device_caps() & (V4L2_CAP_META_CAPTURE |
>> @@ -123,7 +128,7 @@ public:
>>  
>>  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
>>  
>> -	int open();
>> +	int open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);
>>  	void close();
>>  
>>  	const char *driverName() const { return caps_.driver(); }
>> @@ -152,6 +157,9 @@ protected:
>>  	std::string logPrefix() const;
>>  
>>  private:
>> +	int queryBufferType();
>> +	int queryBufferType(enum v4l2_buf_type type);
>> +
>>  	int getFormatMeta(V4L2DeviceFormat *format);
>>  	int setFormatMeta(V4L2DeviceFormat *format);
>>  
>> @@ -182,6 +190,22 @@ private:
>>  	EventNotifier *fdEvent_;
>>  };
>>  
>> +class V4L2M2MDevice
>> +{
>> +public:
>> +	V4L2M2MDevice(const std::string &deviceNode);
>> +	~V4L2M2MDevice();
>> +
>> +	V4L2VideoDevice *output() { return output_; }
>> +	V4L2VideoDevice *capture() { return capture_; }
>> +	unsigned int status() { return status_; }
> 
> The status is an unsigned int, and stores an error value that can be
> negative, so there's a problem.

Ah yes, that should be int. Doesn't matter it's going to be dropped.

> 
> Furthermore, we have no status methods for the other V4L2-related
> classes, so I think the open() should be split out from the constructor
> to make the API consistent.

I'll split it. in that case I'll add close() as well.


>> +
>> +private:
>> +	V4L2VideoDevice *output_;
>> +	V4L2VideoDevice *capture_;
>> +	unsigned int status_;
>> +};
>> +
>>  } /* namespace libcamera */
>>  
>>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index 81098dd70190..4bd33af5f3de 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)
>>   * \return True if the video device can capture or output images
>>   */
>>  
>> +/**
>> + * \fn V4L2Capability::isM2M()
>> + * \brief Identify if the device is an M2M device
>> + * \return True if the device can capture and output images using the M2M API
>> + */
>> +
>>  /**
>>   * \fn V4L2Capability::isMeta()
>>   * \brief Identify if the video device captures or outputs image meta-data
>> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()
>>  	close();
>>  }
>>  
>> +int V4L2VideoDevice::queryBufferType()
>> +{
>> +	if (caps_.isVideoCapture()) {
>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>> +		bufferType_ = caps_.isMultiplanar()
>> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +	} else if (caps_.isVideoOutput()) {
>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>> +		bufferType_ = caps_.isMultiplanar()
>> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> +	} else if (caps_.isMetaCapture()) {
>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>> +		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
>> +	} else if (caps_.isMetaOutput()) {
>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>> +		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
>> +	} else {
>> +		LOG(V4L2, Error) << "Device is not a supported type";
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)
>> +{
>> +	switch (type) {
>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>> +		bufferType_ = caps_.isMultiplanar()
>> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> +		break;
>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>> +		bufferType_ = caps_.isMultiplanar()
>> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +		break;
>> +	default:
>> +		LOG(V4L2, Error) << "Unsupported buffer type";
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> These methods don't rely query the buffer type, they should be renamed.
> The two overloaded version are also a bit confusing :-(

It was the simplest way to satisfy your earlier request to share code.
This segment is the only part which is not shared across the two
possible open() call paths...

>> +
>>  /**
>>   * \brief Open a V4L2 video device and query its capabilities
>> + *
>> + * \param[in] fd The file descriptor to set (optional)
>> + * \param[in] type The device type to operate on (optional)
>> + *
>> + * Opens a device or sets the file descriptor if provided, and then queries the
>> + * capabilities of the device and establishes the buffer types and device events
>> + * accordingly.
>> + *
>>   * \return 0 on success or a negative error code otherwise
>>   */
>> -int V4L2VideoDevice::open()
>> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)
> 
> There's very little code shared between the two implementations of
> open(), I think I would make it two separate methods (both with the same
> name) and remove the default parameter values in the method declaration.
> The code would be easier to read, and you could keep the
> queryBufferType() code inlined.

Ok - back to the RFC version then (with the rename)



>>  {
>>  	int ret;
>>  
>> -	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (fd != -1) {
>> +		ret = V4L2Device::setFd(fd);
>> +		if (ret < 0)
>> +			return ret;
>> +	} else {
>> +		ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>>  
>>  	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
>>  	if (ret < 0) {
>> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()
>>  	 * devices (POLLIN), and write notifications for OUTPUT video devices
>>  	 * (POLLOUT).
>>  	 */
>> -	if (caps_.isVideoCapture()) {
>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>> -		bufferType_ = caps_.isMultiplanar()
>> -			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>> -			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> -	} else if (caps_.isVideoOutput()) {
>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>> -		bufferType_ = caps_.isMultiplanar()
>> -			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>> -			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> -	} else if (caps_.isMetaCapture()) {
>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>> -		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
>> -	} else if (caps_.isMetaOutput()) {
>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>> -		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
>> -	} else {
>> -		LOG(V4L2, Error) << "Device is not a supported type";
>> -		return -EINVAL;
>> -	}
>> +	if (type != V4L2_BUF_TYPE_PRIVATE)
> 
> This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE
> as a magic default value for the non-M2M case is quite a bit of a hack.


It was required from your request to share the code paths, and use the
v4l2_buf_type. It was the only value I could (half-legitimately) use to
prevent getting:

 vimc.cpp:340:19: error: invalid conversion from ‘int’ to
‘v4l2_buf_type’ [-fpermissive]
  if (video_->open())

If the arguments are not overloaded, then we don't need to specify a
default or switch on it.


> 
>> +		queryBufferType(type);
>> +	else
>> +		queryBufferType();
>>  
>>  	fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
>>  	fdEvent_->setEnabled(false);
>> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>>  	return new V4L2VideoDevice(mediaEntity);
>>  }
>>  
>> +/**
>> + * \class V4L2M2MDevice
>> + * \brief Memory2Memory device container
>> + *
>> + * The V4L2M2MDevice manages two V4L2Device instances on the same
> 
> s/V4L2Device/V4L2VideoDevice/
> 
>> + * deviceNode which operate together using two queues to implement the V4L2
> 
> s/deviceNode/device node/
> 
>> + * Memory to Memory API.
>> + *
>> + * V4L2M2MDevices are opened at the point they are created, and will return
> 
> s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the
> plural, otherwise doxygen won't generate links properly)


Indeed.


> s/will //
> 
>> + * both a capture and an output device or neither in the event of failure.
>> + *
>> + * The two devices should be configured and started as normal V4L2Device
> 
> s/should/shall/
> 
>> + * instances. Closing either of the devices will require recreating a new
>> + * V4L2M2MDevice.
> 
> With the proposal to add an open() method, I would add a close() method
> to V4L2M2MDevice that closes both V4L2VideoDevice, and document here
> that the managed V4L2VideoDevice instances shall not be closed manually.

Haha - yes, I'd already come to the close() conclusion as well.


> 
>> + *
>> + * The two V4L2Device instances share a single device node which is opened at
> 
> s/V4L2Device/V4L2VideoDevice/
> 
>> + * the point the V4L2M2MDevice is created.
>> + */
>> +
>> +/**
>> + * \fn V4L2M2MDevice::output
> 
> Missing \brief, and below too.

Will add.

> 
>> + * \return The output V4L2Device instance
>> + */
>> +
>> +/**
>> + * \fn V4L2M2MDevice::capture
>> + * \return The capture V4L2Device instance
>> + */
>> +
>> +/**
>> + * \fn V4L2M2MDevice::status
>> + * \return The construction status of the V4L2M2MDevice
> 
> You should document what the status value contains. Or rather drop this
> method as it won't be needed with open(). Otherwise I'd recommend
> replacing status() with an isValid() method that returns a bool, that
> would make the API more explicit.

I'll go with adding open()/close()


> 
>> + */
>> +
>> +/**
>> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
>> + *
>> + * The deviceNode will be opened and shared across two V4L2VideoDevice
> 
> "will" ? It's done in this method, not later :-)
> 
>> + * instances. One to control the output stream, and one to control the capture
>> + * stream.
> 
> The second sentence has no verb.

Yes, there should be a semi-colon after instances instead of a full-stop.

> 
>> + *
>> + * After construction, the status() must be checked to validate the instance.
>> + */
>> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)
>> +	: status_(0)
>> +{
>> +	int fd[2] = { -1, -1 };
>> +	int ret;
>> +
>> +	output_ = new V4L2VideoDevice(deviceNode);
>> +	capture_ = new V4L2VideoDevice(deviceNode);
>> +
>> +	/* Both V4L2Devices will have the same deviceNode. */
> 
> More than the same device node, they use the same file handle.

Would you consider two handles (after they are dup()d) to be the same
file handle?

Aren't they separate handles representing the same device context which
can have their own lifetime?


> 	/*
> 	 * Both the output and capture V4L2Device instances use the same
> 	 * file handle for the same device node.
> 	 */
> 
>> +	fd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
>> +	if (fd[0] < 0) {
>> +		ret = -errno;
>> +		LOG(V4L2, Error)
>> +			<< "Failed to open V4L2 M2M device: " << strerror(-ret);
>> +		goto err;
>> +	}
>> +
>> +	fd[1] = dup(fd[0]);
>> +	if (fd[1] < 0) {
>> +		ret = -errno;
>> +		LOG(V4L2, Error)
>> +			<< "Failed to duplicate M2M device: " << strerror(-ret);
>> +
>> +		goto err;
>> +	}
>> +
>> +	ret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);
>> +	if (ret)
>> +		goto err;
>> +
>> +	return;
>> +
>> +err:
>> +	delete capture_;
>> +	delete output_;
>> +
>> +	capture_ = nullptr;
>> +	output_ = nullptr;
>> +
>> +	status_ = ret;
>> +
>> +	close(fd[1]);
>> +	close(fd[0]);
> 
> This is racy. delete output_ above will close fd[0] if the
> output_->open() call succeeded, and another thread could open a file
> that reuses the same file descriptor before the close() call here.

I'll move the dup() call to the overloaded open call, and always close
the local handle instead.

>> +}
>> +
>> +V4L2M2MDevice::~V4L2M2MDevice()
>> +{
>> +	delete capture_;
>> +	delete output_;
>> +}
>> +
>>  } /* namespace libcamera */
>

Hi Kieran,

On Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:
> On 08/08/2019 22:03, Laurent Pinchart wrote:
> > On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:
> >> V4L2 M2M devices represent a V4L2Device with two queues. One output, and
> > 
> > s/queues. One/queues: one/ (or "queues, one")
> > 
> >> one capture on a single device node.
> >>
> >> Represent this by instantiating a V4L2VideoDevice for each queue type,
> >> and preparing each device for its queue.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/libcamera/include/v4l2_videodevice.h |  26 ++-
> >>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---
> >>  2 files changed, 201 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> >> index f5c8da93fcb5..24c58d787fde 100644
> >> --- a/src/libcamera/include/v4l2_videodevice.h
> >> +++ b/src/libcamera/include/v4l2_videodevice.h
> >> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {
> >>  					V4L2_CAP_VIDEO_OUTPUT |
> >>  					V4L2_CAP_VIDEO_OUTPUT_MPLANE);
> >>  	}
> >> +	bool isM2M() const
> >> +	{
> >> +		return device_caps() & (V4L2_CAP_VIDEO_M2M |
> >> +					V4L2_CAP_VIDEO_M2M_MPLANE);
> >> +	}
> >>  	bool isMeta() const
> >>  	{
> >>  		return device_caps() & (V4L2_CAP_META_CAPTURE |
> >> @@ -123,7 +128,7 @@ public:
> >>  
> >>  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> >>  
> >> -	int open();
> >> +	int open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);
> >>  	void close();
> >>  
> >>  	const char *driverName() const { return caps_.driver(); }
> >> @@ -152,6 +157,9 @@ protected:
> >>  	std::string logPrefix() const;
> >>  
> >>  private:
> >> +	int queryBufferType();
> >> +	int queryBufferType(enum v4l2_buf_type type);
> >> +
> >>  	int getFormatMeta(V4L2DeviceFormat *format);
> >>  	int setFormatMeta(V4L2DeviceFormat *format);
> >>  
> >> @@ -182,6 +190,22 @@ private:
> >>  	EventNotifier *fdEvent_;
> >>  };
> >>  
> >> +class V4L2M2MDevice
> >> +{
> >> +public:
> >> +	V4L2M2MDevice(const std::string &deviceNode);
> >> +	~V4L2M2MDevice();
> >> +
> >> +	V4L2VideoDevice *output() { return output_; }
> >> +	V4L2VideoDevice *capture() { return capture_; }
> >> +	unsigned int status() { return status_; }
> > 
> > The status is an unsigned int, and stores an error value that can be
> > negative, so there's a problem.
> 
> Ah yes, that should be int. Doesn't matter it's going to be dropped.
> 
> > Furthermore, we have no status methods for the other V4L2-related
> > classes, so I think the open() should be split out from the constructor
> > to make the API consistent.
> 
> I'll split it. in that case I'll add close() as well.
> 
> >> +
> >> +private:
> >> +	V4L2VideoDevice *output_;
> >> +	V4L2VideoDevice *capture_;
> >> +	unsigned int status_;
> >> +};
> >> +
> >>  } /* namespace libcamera */
> >>  
> >>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >> index 81098dd70190..4bd33af5f3de 100644
> >> --- a/src/libcamera/v4l2_videodevice.cpp
> >> +++ b/src/libcamera/v4l2_videodevice.cpp
> >> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)
> >>   * \return True if the video device can capture or output images
> >>   */
> >>  
> >> +/**
> >> + * \fn V4L2Capability::isM2M()
> >> + * \brief Identify if the device is an M2M device
> >> + * \return True if the device can capture and output images using the M2M API
> >> + */
> >> +
> >>  /**
> >>   * \fn V4L2Capability::isMeta()
> >>   * \brief Identify if the video device captures or outputs image meta-data
> >> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()
> >>  	close();
> >>  }
> >>  
> >> +int V4L2VideoDevice::queryBufferType()
> >> +{
> >> +	if (caps_.isVideoCapture()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +	} else if (caps_.isVideoOutput()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> +	} else if (caps_.isMetaCapture()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> +		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> >> +	} else if (caps_.isMetaOutput()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> +		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> >> +	} else {
> >> +		LOG(V4L2, Error) << "Device is not a supported type";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)
> >> +{
> >> +	switch (type) {
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> +		break;
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +		break;
> >> +	default:
> >> +		LOG(V4L2, Error) << "Unsupported buffer type";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > 
> > These methods don't rely query the buffer type, they should be renamed.
> > The two overloaded version are also a bit confusing :-(
> 
> It was the simplest way to satisfy your earlier request to share code.
> This segment is the only part which is not shared across the two
> possible open() call paths...

Yes, I'm sorry. I was hoping code could be shared in a clean way, but
the end result is more difficult to read, and thus not worth it in my
opinion :-( Not your fault of course, just the code's fault :-)

> >> +
> >>  /**
> >>   * \brief Open a V4L2 video device and query its capabilities
> >> + *
> >> + * \param[in] fd The file descriptor to set (optional)
> >> + * \param[in] type The device type to operate on (optional)
> >> + *
> >> + * Opens a device or sets the file descriptor if provided, and then queries the
> >> + * capabilities of the device and establishes the buffer types and device events
> >> + * accordingly.
> >> + *
> >>   * \return 0 on success or a negative error code otherwise
> >>   */
> >> -int V4L2VideoDevice::open()
> >> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)
> > 
> > There's very little code shared between the two implementations of
> > open(), I think I would make it two separate methods (both with the same
> > name) and remove the default parameter values in the method declaration.
> > The code would be easier to read, and you could keep the
> > queryBufferType() code inlined.
> 
> Ok - back to the RFC version then (with the rename)
> 
> >>  {
> >>  	int ret;
> >>  
> >> -	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> >> -	if (ret < 0)
> >> -		return ret;
> >> +	if (fd != -1) {
> >> +		ret = V4L2Device::setFd(fd);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	} else {
> >> +		ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >>  
> >>  	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> >>  	if (ret < 0) {
> >> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()
> >>  	 * devices (POLLIN), and write notifications for OUTPUT video devices
> >>  	 * (POLLOUT).
> >>  	 */
> >> -	if (caps_.isVideoCapture()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> -		bufferType_ = caps_.isMultiplanar()
> >> -			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >> -			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> -	} else if (caps_.isVideoOutput()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> -		bufferType_ = caps_.isMultiplanar()
> >> -			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >> -			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> -	} else if (caps_.isMetaCapture()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> -		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> >> -	} else if (caps_.isMetaOutput()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> -		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> >> -	} else {
> >> -		LOG(V4L2, Error) << "Device is not a supported type";
> >> -		return -EINVAL;
> >> -	}
> >> +	if (type != V4L2_BUF_TYPE_PRIVATE)
> > 
> > This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE
> > as a magic default value for the non-M2M case is quite a bit of a hack.
> 
> It was required from your request to share the code paths, and use the
> v4l2_buf_type. It was the only value I could (half-legitimately) use to
> prevent getting:
> 
>  vimc.cpp:340:19: error: invalid conversion from ‘int’ to
> ‘v4l2_buf_type’ [-fpermissive]
>   if (video_->open())
> 
> If the arguments are not overloaded, then we don't need to specify a
> default or switch on it.
> 
> >> +		queryBufferType(type);
> >> +	else
> >> +		queryBufferType();
> >>  
> >>  	fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
> >>  	fdEvent_->setEnabled(false);
> >> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> >>  	return new V4L2VideoDevice(mediaEntity);
> >>  }
> >>  
> >> +/**
> >> + * \class V4L2M2MDevice
> >> + * \brief Memory2Memory device container
> >> + *
> >> + * The V4L2M2MDevice manages two V4L2Device instances on the same
> > 
> > s/V4L2Device/V4L2VideoDevice/
> > 
> >> + * deviceNode which operate together using two queues to implement the V4L2
> > 
> > s/deviceNode/device node/
> > 
> >> + * Memory to Memory API.
> >> + *
> >> + * V4L2M2MDevices are opened at the point they are created, and will return
> > 
> > s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the
> > plural, otherwise doxygen won't generate links properly)
> 
> Indeed.
> 
> > s/will //
> > 
> >> + * both a capture and an output device or neither in the event of failure.
> >> + *
> >> + * The two devices should be configured and started as normal V4L2Device
> > 
> > s/should/shall/
> > 
> >> + * instances. Closing either of the devices will require recreating a new
> >> + * V4L2M2MDevice.
> > 
> > With the proposal to add an open() method, I would add a close() method
> > to V4L2M2MDevice that closes both V4L2VideoDevice, and document here
> > that the managed V4L2VideoDevice instances shall not be closed manually.
> 
> Haha - yes, I'd already come to the close() conclusion as well.
> 
> > 
> >> + *
> >> + * The two V4L2Device instances share a single device node which is opened at
> > 
> > s/V4L2Device/V4L2VideoDevice/
> > 
> >> + * the point the V4L2M2MDevice is created.
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2M2MDevice::output
> > 
> > Missing \brief, and below too.
> 
> Will add.
> 
> >> + * \return The output V4L2Device instance
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2M2MDevice::capture
> >> + * \return The capture V4L2Device instance
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2M2MDevice::status
> >> + * \return The construction status of the V4L2M2MDevice
> > 
> > You should document what the status value contains. Or rather drop this
> > method as it won't be needed with open(). Otherwise I'd recommend
> > replacing status() with an isValid() method that returns a bool, that
> > would make the API more explicit.
> 
> I'll go with adding open()/close()
> 
> >> + */
> >> +
> >> +/**
> >> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
> >> + *
> >> + * The deviceNode will be opened and shared across two V4L2VideoDevice
> > 
> > "will" ? It's done in this method, not later :-)
> > 
> >> + * instances. One to control the output stream, and one to control the capture
> >> + * stream.
> > 
> > The second sentence has no verb.
> 
> Yes, there should be a semi-colon after instances instead of a full-stop.
> 
> >> + *
> >> + * After construction, the status() must be checked to validate the instance.
> >> + */
> >> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)
> >> +	: status_(0)
> >> +{
> >> +	int fd[2] = { -1, -1 };
> >> +	int ret;
> >> +
> >> +	output_ = new V4L2VideoDevice(deviceNode);
> >> +	capture_ = new V4L2VideoDevice(deviceNode);
> >> +
> >> +	/* Both V4L2Devices will have the same deviceNode. */
> > 
> > More than the same device node, they use the same file handle.
> 
> Would you consider two handles (after they are dup()d) to be the same
> file handle?
> 
> Aren't they separate handles representing the same device context which
> can have their own lifetime?

What's the right word for the middle part of fd -> file -> inode ?
"file" usually refers to a file on disk, and so does device node.
According to man dup,

"After a successful return, the old and new file descriptors may be used
interchangeably.  They refer to the same open file description (see
open(2)) and thus share file offset and file status flags; for example,
if the file offset is modified by using lseek(2) on one of the file
descriptors, the offset is also changed for the other."

"File description" sounds weird.

> > 	/*
> > 	 * Both the output and capture V4L2Device instances use the same
> > 	 * file handle for the same device node.
> > 	 */
> > 
> >> +	fd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
> >> +	if (fd[0] < 0) {
> >> +		ret = -errno;
> >> +		LOG(V4L2, Error)
> >> +			<< "Failed to open V4L2 M2M device: " << strerror(-ret);
> >> +		goto err;
> >> +	}
> >> +
> >> +	fd[1] = dup(fd[0]);
> >> +	if (fd[1] < 0) {
> >> +		ret = -errno;
> >> +		LOG(V4L2, Error)
> >> +			<< "Failed to duplicate M2M device: " << strerror(-ret);
> >> +
> >> +		goto err;
> >> +	}
> >> +
> >> +	ret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
> >> +	if (ret)
> >> +		goto err;
> >> +
> >> +	ret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);
> >> +	if (ret)
> >> +		goto err;
> >> +
> >> +	return;
> >> +
> >> +err:
> >> +	delete capture_;
> >> +	delete output_;
> >> +
> >> +	capture_ = nullptr;
> >> +	output_ = nullptr;
> >> +
> >> +	status_ = ret;
> >> +
> >> +	close(fd[1]);
> >> +	close(fd[0]);
> > 
> > This is racy. delete output_ above will close fd[0] if the
> > output_->open() call succeeded, and another thread could open a file
> > that reuses the same file descriptor before the close() call here.
> 
> I'll move the dup() call to the overloaded open call, and always close
> the local handle instead.

Good idea !

The other option would be to skip close() on the fd in V4L2Device if the
fd was set with setFd(), and close it in V4L2M2MDevice::close(). This
couldn't be done before as there was no close() for V4L2M2MDevice. What
do you think would be best ?

> >> +}
> >> +
> >> +V4L2M2MDevice::~V4L2M2MDevice()
> >> +{
> >> +	delete capture_;
> >> +	delete output_;
> >> +}
> >> +
> >>  } /* namespace libcamera */

Hi Kieran,

On Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:
> Hi Laurent,
>
> On 08/08/2019 22:03, Laurent Pinchart wrote:
> > Hi Kieran,
> >
> > Thank you for the patch.

I don't have other comments than Laurent's one, but...

> >
> > On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:
> >> V4L2 M2M devices represent a V4L2Device with two queues. One output, and
> >
> > s/queues. One/queues: one/ (or "queues, one")
> >
> >> one capture on a single device node.
> >>
> >> Represent this by instantiating a V4L2VideoDevice for each queue type,
> >> and preparing each device for its queue.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/libcamera/include/v4l2_videodevice.h |  26 ++-
> >>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---
> >>  2 files changed, 201 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> >> index f5c8da93fcb5..24c58d787fde 100644
> >> --- a/src/libcamera/include/v4l2_videodevice.h
> >> +++ b/src/libcamera/include/v4l2_videodevice.h
> >> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {
> >>  					V4L2_CAP_VIDEO_OUTPUT |
> >>  					V4L2_CAP_VIDEO_OUTPUT_MPLANE);
> >>  	}
> >> +	bool isM2M() const
> >> +	{
> >> +		return device_caps() & (V4L2_CAP_VIDEO_M2M |
> >> +					V4L2_CAP_VIDEO_M2M_MPLANE);
> >> +	}
> >>  	bool isMeta() const
> >>  	{
> >>  		return device_caps() & (V4L2_CAP_META_CAPTURE |
> >> @@ -123,7 +128,7 @@ public:
> >>
> >>  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> >>
> >> -	int open();
> >> +	int open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);
> >>  	void close();
> >>
> >>  	const char *driverName() const { return caps_.driver(); }
> >> @@ -152,6 +157,9 @@ protected:
> >>  	std::string logPrefix() const;
> >>
> >>  private:
> >> +	int queryBufferType();
> >> +	int queryBufferType(enum v4l2_buf_type type);
> >> +
> >>  	int getFormatMeta(V4L2DeviceFormat *format);
> >>  	int setFormatMeta(V4L2DeviceFormat *format);
> >>
> >> @@ -182,6 +190,22 @@ private:
> >>  	EventNotifier *fdEvent_;
> >>  };
> >>
> >> +class V4L2M2MDevice
> >> +{
> >> +public:
> >> +	V4L2M2MDevice(const std::string &deviceNode);
> >> +	~V4L2M2MDevice();
> >> +
> >> +	V4L2VideoDevice *output() { return output_; }
> >> +	V4L2VideoDevice *capture() { return capture_; }
> >> +	unsigned int status() { return status_; }
> >
> > The status is an unsigned int, and stores an error value that can be
> > negative, so there's a problem.
>
> Ah yes, that should be int. Doesn't matter it's going to be dropped.
>
> >
> > Furthermore, we have no status methods for the other V4L2-related
> > classes, so I think the open() should be split out from the constructor
> > to make the API consistent.
>
> I'll split it. in that case I'll add close() as well.
>
>
> >> +
> >> +private:
> >> +	V4L2VideoDevice *output_;
> >> +	V4L2VideoDevice *capture_;
> >> +	unsigned int status_;
> >> +};
> >> +
> >>  } /* namespace libcamera */
> >>
> >>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >> index 81098dd70190..4bd33af5f3de 100644
> >> --- a/src/libcamera/v4l2_videodevice.cpp
> >> +++ b/src/libcamera/v4l2_videodevice.cpp
> >> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)
> >>   * \return True if the video device can capture or output images
> >>   */
> >>
> >> +/**
> >> + * \fn V4L2Capability::isM2M()
> >> + * \brief Identify if the device is an M2M device
> >> + * \return True if the device can capture and output images using the M2M API
> >> + */
> >> +
> >>  /**
> >>   * \fn V4L2Capability::isMeta()
> >>   * \brief Identify if the video device captures or outputs image meta-data
> >> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()
> >>  	close();
> >>  }
> >>
> >> +int V4L2VideoDevice::queryBufferType()
> >> +{
> >> +	if (caps_.isVideoCapture()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +	} else if (caps_.isVideoOutput()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> +	} else if (caps_.isMetaCapture()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> +		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> >> +	} else if (caps_.isMetaOutput()) {
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> +		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> >> +	} else {
> >> +		LOG(V4L2, Error) << "Device is not a supported type";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)
> >> +{
> >> +	switch (type) {
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> +		break;
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> +		bufferType_ = caps_.isMultiplanar()
> >> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +		break;
> >> +	default:
> >> +		LOG(V4L2, Error) << "Unsupported buffer type";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >
> > These methods don't rely query the buffer type, they should be renamed.
> > The two overloaded version are also a bit confusing :-(
>
> It was the simplest way to satisfy your earlier request to share code.
> This segment is the only part which is not shared across the two
> possible open() call paths...
>
> >> +
> >>  /**
> >>   * \brief Open a V4L2 video device and query its capabilities
> >> + *
> >> + * \param[in] fd The file descriptor to set (optional)
> >> + * \param[in] type The device type to operate on (optional)
> >> + *
> >> + * Opens a device or sets the file descriptor if provided, and then queries the
> >> + * capabilities of the device and establishes the buffer types and device events
> >> + * accordingly.
> >> + *
> >>   * \return 0 on success or a negative error code otherwise
> >>   */
> >> -int V4L2VideoDevice::open()
> >> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)
> >
> > There's very little code shared between the two implementations of
> > open(), I think I would make it two separate methods (both with the same
> > name) and remove the default parameter values in the method declaration.
> > The code would be easier to read, and you could keep the
> > queryBufferType() code inlined.
>
> Ok - back to the RFC version then (with the rename)

I too find quite confusing using the default arguments to
differentiate between the use cases. I mean, not weird, but hard to
follow. I would overload the operation or either make a new one for
the purpose and open code queryBufferType there.

Thanks
   j

>
>
>
> >>  {
> >>  	int ret;
> >>
> >> -	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> >> -	if (ret < 0)
> >> -		return ret;
> >> +	if (fd != -1) {
> >> +		ret = V4L2Device::setFd(fd);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	} else {
> >> +		ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >>
> >>  	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> >>  	if (ret < 0) {
> >> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()
> >>  	 * devices (POLLIN), and write notifications for OUTPUT video devices
> >>  	 * (POLLOUT).
> >>  	 */
> >> -	if (caps_.isVideoCapture()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> -		bufferType_ = caps_.isMultiplanar()
> >> -			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >> -			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> -	} else if (caps_.isVideoOutput()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> -		bufferType_ = caps_.isMultiplanar()
> >> -			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >> -			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> -	} else if (caps_.isMetaCapture()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >> -		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> >> -	} else if (caps_.isMetaOutput()) {
> >> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >> -		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> >> -	} else {
> >> -		LOG(V4L2, Error) << "Device is not a supported type";
> >> -		return -EINVAL;
> >> -	}
> >> +	if (type != V4L2_BUF_TYPE_PRIVATE)
> >
> > This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE
> > as a magic default value for the non-M2M case is quite a bit of a hack.
>
>
> It was required from your request to share the code paths, and use the
> v4l2_buf_type. It was the only value I could (half-legitimately) use to
> prevent getting:
>
>  vimc.cpp:340:19: error: invalid conversion from ‘int’ to
> ‘v4l2_buf_type’ [-fpermissive]
>   if (video_->open())
>
> If the arguments are not overloaded, then we don't need to specify a
> default or switch on it.
>
>
> >
> >> +		queryBufferType(type);
> >> +	else
> >> +		queryBufferType();
> >>
> >>  	fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
> >>  	fdEvent_->setEnabled(false);
> >> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> >>  	return new V4L2VideoDevice(mediaEntity);
> >>  }
> >>
> >> +/**
> >> + * \class V4L2M2MDevice
> >> + * \brief Memory2Memory device container
> >> + *
> >> + * The V4L2M2MDevice manages two V4L2Device instances on the same
> >
> > s/V4L2Device/V4L2VideoDevice/
> >
> >> + * deviceNode which operate together using two queues to implement the V4L2
> >
> > s/deviceNode/device node/
> >
> >> + * Memory to Memory API.
> >> + *
> >> + * V4L2M2MDevices are opened at the point they are created, and will return
> >
> > s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the
> > plural, otherwise doxygen won't generate links properly)
>
>
> Indeed.
>
>
> > s/will //
> >
> >> + * both a capture and an output device or neither in the event of failure.
> >> + *
> >> + * The two devices should be configured and started as normal V4L2Device
> >
> > s/should/shall/
> >
> >> + * instances. Closing either of the devices will require recreating a new
> >> + * V4L2M2MDevice.
> >
> > With the proposal to add an open() method, I would add a close() method
> > to V4L2M2MDevice that closes both V4L2VideoDevice, and document here
> > that the managed V4L2VideoDevice instances shall not be closed manually.
>
> Haha - yes, I'd already come to the close() conclusion as well.
>
>
> >
> >> + *
> >> + * The two V4L2Device instances share a single device node which is opened at
> >
> > s/V4L2Device/V4L2VideoDevice/
> >
> >> + * the point the V4L2M2MDevice is created.
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2M2MDevice::output
> >
> > Missing \brief, and below too.
>
> Will add.
>
> >
> >> + * \return The output V4L2Device instance
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2M2MDevice::capture
> >> + * \return The capture V4L2Device instance
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2M2MDevice::status
> >> + * \return The construction status of the V4L2M2MDevice
> >
> > You should document what the status value contains. Or rather drop this
> > method as it won't be needed with open(). Otherwise I'd recommend
> > replacing status() with an isValid() method that returns a bool, that
> > would make the API more explicit.
>
> I'll go with adding open()/close()
>
>
> >
> >> + */
> >> +
> >> +/**
> >> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
> >> + *
> >> + * The deviceNode will be opened and shared across two V4L2VideoDevice
> >
> > "will" ? It's done in this method, not later :-)
> >
> >> + * instances. One to control the output stream, and one to control the capture
> >> + * stream.
> >
> > The second sentence has no verb.
>
> Yes, there should be a semi-colon after instances instead of a full-stop.
>
> >
> >> + *
> >> + * After construction, the status() must be checked to validate the instance.
> >> + */
> >> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)
> >> +	: status_(0)
> >> +{
> >> +	int fd[2] = { -1, -1 };
> >> +	int ret;
> >> +
> >> +	output_ = new V4L2VideoDevice(deviceNode);
> >> +	capture_ = new V4L2VideoDevice(deviceNode);
> >> +
> >> +	/* Both V4L2Devices will have the same deviceNode. */
> >
> > More than the same device node, they use the same file handle.
>
> Would you consider two handles (after they are dup()d) to be the same
> file handle?
>
> Aren't they separate handles representing the same device context which
> can have their own lifetime?
>
>
> > 	/*
> > 	 * Both the output and capture V4L2Device instances use the same
> > 	 * file handle for the same device node.
> > 	 */
> >
> >> +	fd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
> >> +	if (fd[0] < 0) {
> >> +		ret = -errno;
> >> +		LOG(V4L2, Error)
> >> +			<< "Failed to open V4L2 M2M device: " << strerror(-ret);
> >> +		goto err;
> >> +	}
> >> +
> >> +	fd[1] = dup(fd[0]);
> >> +	if (fd[1] < 0) {
> >> +		ret = -errno;
> >> +		LOG(V4L2, Error)
> >> +			<< "Failed to duplicate M2M device: " << strerror(-ret);
> >> +
> >> +		goto err;
> >> +	}
> >> +
> >> +	ret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
> >> +	if (ret)
> >> +		goto err;
> >> +
> >> +	ret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);
> >> +	if (ret)
> >> +		goto err;
> >> +
> >> +	return;
> >> +
> >> +err:
> >> +	delete capture_;
> >> +	delete output_;
> >> +
> >> +	capture_ = nullptr;
> >> +	output_ = nullptr;
> >> +
> >> +	status_ = ret;
> >> +
> >> +	close(fd[1]);
> >> +	close(fd[0]);
> >
> > This is racy. delete output_ above will close fd[0] if the
> > output_->open() call succeeded, and another thread could open a file
> > that reuses the same file descriptor before the close() call here.
>
> I'll move the dup() call to the overloaded open call, and always close
> the local handle instead.
>
> >> +}
> >> +
> >> +V4L2M2MDevice::~V4L2M2MDevice()
> >> +{
> >> +	delete capture_;
> >> +	delete output_;
> >> +}
> >> +
> >>  } /* namespace libcamera */
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

On 09/08/2019 09:25, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:
>> On 08/08/2019 22:03, Laurent Pinchart wrote:
>>> On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:
>>>> V4L2 M2M devices represent a V4L2Device with two queues. One output, and
>>>
>>> s/queues. One/queues: one/ (or "queues, one")
>>>

Done

>>>> one capture on a single device node.
>>>>
>>>> Represent this by instantiating a V4L2VideoDevice for each queue type,
>>>> and preparing each device for its queue.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>  src/libcamera/include/v4l2_videodevice.h |  26 ++-
>>>>  src/libcamera/v4l2_videodevice.cpp       | 200 ++++++++++++++++++++---
>>>>  2 files changed, 201 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
>>>> index f5c8da93fcb5..24c58d787fde 100644
>>>> --- a/src/libcamera/include/v4l2_videodevice.h
>>>> +++ b/src/libcamera/include/v4l2_videodevice.h
>>>> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {
>>>>  					V4L2_CAP_VIDEO_OUTPUT |
>>>>  					V4L2_CAP_VIDEO_OUTPUT_MPLANE);
>>>>  	}
>>>> +	bool isM2M() const
>>>> +	{
>>>> +		return device_caps() & (V4L2_CAP_VIDEO_M2M |
>>>> +					V4L2_CAP_VIDEO_M2M_MPLANE);
>>>> +	}
>>>>  	bool isMeta() const
>>>>  	{
>>>>  		return device_caps() & (V4L2_CAP_META_CAPTURE |
>>>> @@ -123,7 +128,7 @@ public:
>>>>  
>>>>  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
>>>>  
>>>> -	int open();
>>>> +	int open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);
>>>>  	void close();
>>>>  
>>>>  	const char *driverName() const { return caps_.driver(); }
>>>> @@ -152,6 +157,9 @@ protected:
>>>>  	std::string logPrefix() const;
>>>>  
>>>>  private:
>>>> +	int queryBufferType();
>>>> +	int queryBufferType(enum v4l2_buf_type type);
>>>> +
>>>>  	int getFormatMeta(V4L2DeviceFormat *format);
>>>>  	int setFormatMeta(V4L2DeviceFormat *format);
>>>>  
>>>> @@ -182,6 +190,22 @@ private:
>>>>  	EventNotifier *fdEvent_;
>>>>  };
>>>>  
>>>> +class V4L2M2MDevice
>>>> +{
>>>> +public:
>>>> +	V4L2M2MDevice(const std::string &deviceNode);
>>>> +	~V4L2M2MDevice();
>>>> +
>>>> +	V4L2VideoDevice *output() { return output_; }
>>>> +	V4L2VideoDevice *capture() { return capture_; }
>>>> +	unsigned int status() { return status_; }
>>>
>>> The status is an unsigned int, and stores an error value that can be
>>> negative, so there's a problem.
>>
>> Ah yes, that should be int. Doesn't matter it's going to be dropped.
>>
>>> Furthermore, we have no status methods for the other V4L2-related
>>> classes, so I think the open() should be split out from the constructor
>>> to make the API consistent.
>>
>> I'll split it. in that case I'll add close() as well.

Split done.



>>
>>>> +
>>>> +private:
>>>> +	V4L2VideoDevice *output_;
>>>> +	V4L2VideoDevice *capture_;
>>>> +	unsigned int status_;
>>>> +};
>>>> +
>>>>  } /* namespace libcamera */
>>>>  
>>>>  #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>>>> index 81098dd70190..4bd33af5f3de 100644
>>>> --- a/src/libcamera/v4l2_videodevice.cpp
>>>> +++ b/src/libcamera/v4l2_videodevice.cpp
>>>> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)
>>>>   * \return True if the video device can capture or output images
>>>>   */
>>>>  
>>>> +/**
>>>> + * \fn V4L2Capability::isM2M()
>>>> + * \brief Identify if the device is an M2M device
>>>> + * \return True if the device can capture and output images using the M2M API
>>>> + */
>>>> +
>>>>  /**
>>>>   * \fn V4L2Capability::isMeta()
>>>>   * \brief Identify if the video device captures or outputs image meta-data
>>>> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()
>>>>  	close();
>>>>  }
>>>>  
>>>> +int V4L2VideoDevice::queryBufferType()
>>>> +{
>>>> +	if (caps_.isVideoCapture()) {
>>>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>>>> +		bufferType_ = caps_.isMultiplanar()
>>>> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>>>> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>>> +	} else if (caps_.isVideoOutput()) {
>>>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>>>> +		bufferType_ = caps_.isMultiplanar()
>>>> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>>>> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>>>> +	} else if (caps_.isMetaCapture()) {
>>>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>>>> +		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
>>>> +	} else if (caps_.isMetaOutput()) {
>>>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>>>> +		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
>>>> +	} else {
>>>> +		LOG(V4L2, Error) << "Device is not a supported type";
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)
>>>> +{
>>>> +	switch (type) {
>>>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>>>> +		bufferType_ = caps_.isMultiplanar()
>>>> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>>>> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>>>> +		break;
>>>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>>> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>>>> +		bufferType_ = caps_.isMultiplanar()
>>>> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>>>> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>>> +		break;
>>>> +	default:
>>>> +		LOG(V4L2, Error) << "Unsupported buffer type";
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> These methods don't rely query the buffer type, they should be renamed.
>>> The two overloaded version are also a bit confusing :-(
>>
>> It was the simplest way to satisfy your earlier request to share code.
>> This segment is the only part which is not shared across the two
>> possible open() call paths...
> 
> Yes, I'm sorry. I was hoping code could be shared in a clean way, but
> the end result is more difficult to read, and thus not worth it in my
> opinion :-( Not your fault of course, just the code's fault :-)

Ok - I've gone back to two open() calls. One for normal code paths, and
one for the M2M code path.



> 
>>>> +
>>>>  /**
>>>>   * \brief Open a V4L2 video device and query its capabilities
>>>> + *
>>>> + * \param[in] fd The file descriptor to set (optional)
>>>> + * \param[in] type The device type to operate on (optional)
>>>> + *
>>>> + * Opens a device or sets the file descriptor if provided, and then queries the
>>>> + * capabilities of the device and establishes the buffer types and device events
>>>> + * accordingly.
>>>> + *
>>>>   * \return 0 on success or a negative error code otherwise
>>>>   */
>>>> -int V4L2VideoDevice::open()
>>>> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)
>>>
>>> There's very little code shared between the two implementations of
>>> open(), I think I would make it two separate methods (both with the same
>>> name) and remove the default parameter values in the method declaration.
>>> The code would be easier to read, and you could keep the
>>> queryBufferType() code inlined.
>>
>> Ok - back to the RFC version then (with the rename)
>>
>>>>  {
>>>>  	int ret;
>>>>  
>>>> -	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>> +	if (fd != -1) {
>>>> +		ret = V4L2Device::setFd(fd);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +	} else {
>>>> +		ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +	}
>>>>  
>>>>  	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
>>>>  	if (ret < 0) {
>>>> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()
>>>>  	 * devices (POLLIN), and write notifications for OUTPUT video devices
>>>>  	 * (POLLOUT).
>>>>  	 */
>>>> -	if (caps_.isVideoCapture()) {
>>>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>>>> -		bufferType_ = caps_.isMultiplanar()
>>>> -			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>>>> -			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>>> -	} else if (caps_.isVideoOutput()) {
>>>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>>>> -		bufferType_ = caps_.isMultiplanar()
>>>> -			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>>>> -			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>>>> -	} else if (caps_.isMetaCapture()) {
>>>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>>>> -		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
>>>> -	} else if (caps_.isMetaOutput()) {
>>>> -		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>>>> -		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
>>>> -	} else {
>>>> -		LOG(V4L2, Error) << "Device is not a supported type";
>>>> -		return -EINVAL;
>>>> -	}
>>>> +	if (type != V4L2_BUF_TYPE_PRIVATE)
>>>
>>> This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE
>>> as a magic default value for the non-M2M case is quite a bit of a hack.
>>
>> It was required from your request to share the code paths, and use the
>> v4l2_buf_type. It was the only value I could (half-legitimately) use to
>> prevent getting:
>>
>>  vimc.cpp:340:19: error: invalid conversion from ‘int’ to
>> ‘v4l2_buf_type’ [-fpermissive]
>>   if (video_->open())
>>
>> If the arguments are not overloaded, then we don't need to specify a
>> default or switch on it.
>>
>>>> +		queryBufferType(type);
>>>> +	else
>>>> +		queryBufferType();
>>>>  
>>>>  	fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
>>>>  	fdEvent_->setEnabled(false);
>>>> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>>>>  	return new V4L2VideoDevice(mediaEntity);
>>>>  }
>>>>  
>>>> +/**
>>>> + * \class V4L2M2MDevice
>>>> + * \brief Memory2Memory device container
>>>> + *
>>>> + * The V4L2M2MDevice manages two V4L2Device instances on the same
>>>
>>> s/V4L2Device/V4L2VideoDevice/
>>>
>>>> + * deviceNode which operate together using two queues to implement the V4L2
>>>
>>> s/deviceNode/device node/
>>>
>>>> + * Memory to Memory API.
>>>> + *
>>>> + * V4L2M2MDevices are opened at the point they are created, and will return
>>>
>>> s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the
>>> plural, otherwise doxygen won't generate links properly)
>>
>> Indeed.
>>
>>> s/will //
>>>
>>>> + * both a capture and an output device or neither in the event of failure.
>>>> + *
>>>> + * The two devices should be configured and started as normal V4L2Device
>>>
>>> s/should/shall/
>>>
>>>> + * instances. Closing either of the devices will require recreating a new
>>>> + * V4L2M2MDevice.
>>>
>>> With the proposal to add an open() method, I would add a close() method
>>> to V4L2M2MDevice that closes both V4L2VideoDevice, and document here
>>> that the managed V4L2VideoDevice instances shall not be closed manually.
>>
>> Haha - yes, I'd already come to the close() conclusion as well.
>>
>>>
>>>> + *
>>>> + * The two V4L2Device instances share a single device node which is opened at
>>>
>>> s/V4L2Device/V4L2VideoDevice/
>>>
>>>> + * the point the V4L2M2MDevice is created.
>>>> + */
>>>> +
>>>> +/**
>>>> + * \fn V4L2M2MDevice::output
>>>
>>> Missing \brief, and below too.
>>
>> Will add.
>>
>>>> + * \return The output V4L2Device instance
>>>> + */
>>>> +
>>>> +/**
>>>> + * \fn V4L2M2MDevice::capture
>>>> + * \return The capture V4L2Device instance
>>>> + */
>>>> +
>>>> +/**
>>>> + * \fn V4L2M2MDevice::status
>>>> + * \return The construction status of the V4L2M2MDevice
>>>
>>> You should document what the status value contains. Or rather drop this
>>> method as it won't be needed with open(). Otherwise I'd recommend
>>> replacing status() with an isValid() method that returns a bool, that
>>> would make the API more explicit.
>>
>> I'll go with adding open()/close()
>>
>>>> + */
>>>> +
>>>> +/**
>>>> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
>>>> + *
>>>> + * The deviceNode will be opened and shared across two V4L2VideoDevice
>>>
>>> "will" ? It's done in this method, not later :-)
>>>
>>>> + * instances. One to control the output stream, and one to control the capture
>>>> + * stream.
>>>
>>> The second sentence has no verb.
>>
>> Yes, there should be a semi-colon after instances instead of a full-stop.
>>
>>>> + *
>>>> + * After construction, the status() must be checked to validate the instance.
>>>> + */
>>>> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)
>>>> +	: status_(0)
>>>> +{
>>>> +	int fd[2] = { -1, -1 };
>>>> +	int ret;
>>>> +
>>>> +	output_ = new V4L2VideoDevice(deviceNode);
>>>> +	capture_ = new V4L2VideoDevice(deviceNode);
>>>> +
>>>> +	/* Both V4L2Devices will have the same deviceNode. */
>>>
>>> More than the same device node, they use the same file handle.
>>
>> Would you consider two handles (after they are dup()d) to be the same
>> file handle?
>>
>> Aren't they separate handles representing the same device context which
>> can have their own lifetime?
> 
> What's the right word for the middle part of fd -> file -> inode ?
> "file" usually refers to a file on disk, and so does device node.
> According to man dup,
> 
> "After a successful return, the old and new file descriptors may be used
> interchangeably.  They refer to the same open file description (see
> open(2)) and thus share file offset and file status flags; for example,
> if the file offset is modified by using lseek(2) on one of the file
> descriptors, the offset is also changed for the other."
> 
> "File description" sounds weird.

I've added

/*
 * The output and capture V4L2VideoDevice instances use the same file
 * handle for the same device node. The local file handle can be closed
 * as the V4L2VideoDevice::open() retains a handle by duplicating the
 * fd passed in.
 */

(which is correct in v2, where the dup happens in the open(fd, type) call)


> 
>>> 	/*
>>> 	 * Both the output and capture V4L2Device instances use the same
>>> 	 * file handle for the same device node.
>>> 	 */
>>>
>>>> +	fd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
>>>> +	if (fd[0] < 0) {
>>>> +		ret = -errno;
>>>> +		LOG(V4L2, Error)
>>>> +			<< "Failed to open V4L2 M2M device: " << strerror(-ret);
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	fd[1] = dup(fd[0]);
>>>> +	if (fd[1] < 0) {
>>>> +		ret = -errno;
>>>> +		LOG(V4L2, Error)
>>>> +			<< "Failed to duplicate M2M device: " << strerror(-ret);
>>>> +
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	ret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
>>>> +	if (ret)
>>>> +		goto err;
>>>> +
>>>> +	ret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);
>>>> +	if (ret)
>>>> +		goto err;
>>>> +
>>>> +	return;
>>>> +
>>>> +err:
>>>> +	delete capture_;
>>>> +	delete output_;
>>>> +
>>>> +	capture_ = nullptr;
>>>> +	output_ = nullptr;
>>>> +
>>>> +	status_ = ret;
>>>> +
>>>> +	close(fd[1]);
>>>> +	close(fd[0]);
>>>
>>> This is racy. delete output_ above will close fd[0] if the
>>> output_->open() call succeeded, and another thread could open a file
>>> that reuses the same file descriptor before the close() call here.
>>
>> I'll move the dup() call to the overloaded open call, and always close
>> the local handle instead.
> 
> Good idea !
> 
> The other option would be to skip close() on the fd in V4L2Device if the
> fd was set with setFd(), and close it in V4L2M2MDevice::close(). This
> couldn't be done before as there was no close() for V4L2M2MDevice. What
> do you think would be best ?

I prefer handling the dup in the open call so that each device clearly
'takes' it's own handle.

And that's what I've done.

V2 post imminent.



>>>> +}
>>>> +
>>>> +V4L2M2MDevice::~V4L2M2MDevice()
>>>> +{
>>>> +	delete capture_;
>>>> +	delete output_;
>>>> +}
>>>> +
>>>>  } /* namespace libcamera */
>