[libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Support M2M devices

[Kieran Bingham]

Hi Laurent,

On 04/05/2019 15:47, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, May 03, 2019 at 04:39:51PM +0100, Kieran Bingham wrote:
>> V4L2 M2M devices represent a V4L2Device with two queues. One output, and
>> one capture on a single device node.
>>
>> Represent this by instantiating a V4L2Device for each queue type, and
>> preparing each device for its queue.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>
>> Marking this patch as RFC as yet it does not have a user or test case associated with it.
>>
>> However this patch is part of a development to support a RaspberryPi pipeline
>> handler and is sent independently for some early review and bikeshedding :-)
>>
>> It has been used successfully to interact with the RaspberryPi ISP M2M driver.
>>
>>  src/libcamera/include/v4l2_device.h |  13 +++
>>  src/libcamera/v4l2_device.cpp       | 159 ++++++++++++++++++++++++++++
>>  2 files changed, 172 insertions(+)
>>
>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>> index 689e18dea7a6..4eaf140f452f 100644
>> --- a/src/libcamera/include/v4l2_device.h
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -62,6 +62,11 @@ struct V4L2Capability final : v4l2_capability {
>>  		return device_caps() & (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 isVideo() const
>>  	{
>>  		return device_caps() & (V4L2_CAP_VIDEO_CAPTURE |
>> @@ -117,6 +122,7 @@ public:
>>  	V4L2Device &operator=(const V4L2Device &) = delete;
>>  
>>  	int open();
>> +	int setFD(int fd, int type);
> 
> s/setFD/setFd/ ?

Done.

> 
> I would replace int type with an enum to make the API clearer.
> v4l2_buf_type is a candidate. Using the V4L2M2MDeviceType enum defined
> below would make this function specific to M2M, while I see no reason
> not to make it generic.

I'm not sure I fully understand you here.
Do you mean you would see


int V4L2VideoDevice::setFd(int fd, enum v4l2_buf_type type)
{
}

Where we call with:

	ret = m2m.output->setFd(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
	ret = m2m.capture->setFd(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);

?

Doesn't that then lead people to think they could call with arbitrary
valid V4L2_BUF_TYPE_VIDEO_xxx types which would not be the case?



>>  	bool isOpen() const;
>>  	void close();
>>  
>> @@ -178,6 +184,13 @@ private:
>>  	EventNotifier *fdEvent_;
>>  };
>>  
>> +struct V4L2M2MDevice {
>> +	V4L2Device *output;
>> +	V4L2Device *capture;
>> +};
>> +
>> +struct V4L2M2MDevice createM2MPair(const std::string &deviceNode);
>> +
>>  } /* namespace libcamera */
>>  
>>  #endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 916f0360503f..cba1c1799596 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -110,6 +110,12 @@ LOG_DEFINE_CATEGORY(V4L2)
>>   * \return True if the device can 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::isMetaCapture()
>>   * \brief Identify if the device captures image meta-data
>> @@ -359,6 +365,74 @@ int V4L2Device::open()
>>  	return 0;
>>  }
>>  
>> +enum V4L2M2MDeviceType {
>> +	V4L2M2MOutput,
>> +	V4L2M2MCapture,
>> +};
>> +
>> +/**
>> + * \brief Open a V4L2 M2M device for a specific queue type using an existing fd
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int V4L2Device::setFD(int fd, int type)
>> +{
>> +	int ret;
>> +
>> +	fd_ = fd;
>> +
>> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
>> +	if (ret < 0) {
>> +		ret = -errno;
>> +		LOG(V4L2, Error)
>> +			<< "Failed to query device capabilities: "
>> +			<< strerror(-ret);
>> +		return ret;
>> +	}
>> +
>> +	LOG(V4L2, Debug)
>> +		<< "Opened device" << caps_.bus_info() << ": "
>> +		<< caps_.driver() << ": " << caps_.card();
>> +
>> +	if (!caps_.isM2M()) {
>> +		LOG(V4L2, Error) << "Device is not an M2M device";
>> +		return -EINVAL;
>> +	}
>> +
> 
> Is this a problem ?

SetFd is only used by M2M devices. Other devices should use open()

I've looked through and I really can't see a good way of making open()
use SetFd effectively.

It's not really about setting the Fd so we could rename the function
instead. It's an alternate open() call really. (Where open() does more
than just call ::open() anyway )

I'm 'open' to suggestions on the name :D

how about calling this openM2M() (even though it doesn't do the open())

otherwise we could call it queryM2M(), but that would suggest that
open() should be called query() ... :S

> 
>> +	if (!caps_.hasStreaming()) {
>> +		LOG(V4L2, Error) << "Device does not support streaming I/O";
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Set buffer type and wait for read notifications on CAPTURE devices
>> +	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
>> +	 */
>> +	switch(type) {
>> +	case V4L2M2MOutput:
>> +		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
>> +		bufferType_ = caps_.isMultiplanar()
>> +			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>> +			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> +		break;
>> +	case V4L2M2MCapture:
>> +		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
>> +		bufferType_ = caps_.isMultiplanar()
>> +			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>> +			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +
>> +		break;
>> +	default:
>> +		LOG(V4L2, Debug) << "M2M type is not supported";
>> +		return -EINVAL;
>> +	}
> 
> Could we share some of the code with open() ? MAybe open() should call
> setFd() ?


This doesn't seem to work well. This switch statement is distinctly
different from the one in open, and the two calls {open(), setFd()}
revolve heavily around the decisions made here.

I can't see a clean way to share code between the two code paths that
would be easier to maintain. (other than making several smaller
functions which I'm not sure would be better)

Feel free to propose something if you want the code paths altered.


>> +
>> +	fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable);
>> +	fdEvent_->setEnabled(false);
>> +
>> +	return 0;
>> +}
>> +
>> +

Extra line to remove there. <done>


>>  /**
>>   * \brief Check if device is successfully opened
>>   * \return True if the device is open, false otherwise
>> @@ -1049,4 +1123,89 @@ V4L2Device *V4L2Device::fromEntityName(const MediaDevice *media,
>>  	return new V4L2Device(mediaEntity);
>>  }
>>  
>> +/**
>> + * \struct V4L2M2MDevice
>> + * \brief Memory2Memory device container
>> + *
>> + * The V4L2M2MDevice structure manages two V4L2Device instances on the same
>> + * deviceNode which operate together using two queues to implement the V4L2
>> + * Memory to Memory API.
>> + *
>> + * V4L2M2MDevices are opened at the point they are created, and will return
>> + * both a capture and an output device or neither in the event of failure.
>> + *
>> + * The two devices should be configured and started as a normal V4L2Device.
> 
> s/a normal V4L2Device/normal V4L2Device instances/
> 
>> + * Closing the device will require recreating a new pair.
> 
> "Closing any of the devices" ? I think you should expand this a bit to
> explain that the open() call isn't valid on a device created this way.
> 
>> + */
>> +
>> +/**
>> + * \var V4L2M2MDevice::output
>> + * \brief The output V4L2Device to send images to
>> + */
>> +
>> +/**
>> + * \var V4L2M2MDevice::capture
>> + * \brief The capture V4L2Device to receive processed images from
>> + */
>> +
>> +/**
>> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
>> + *
>> + * \return a new instantiation
> 
> This should be expanded a bit.
> 
>> + */
>> +struct V4L2M2MDevice createM2MPair(const std::string &deviceNode)
>> +{
>> +	V4L2M2MDevice m2m;
>> +	int ret;
>> +	int fd[2];
>> +
>> +	/* Both V4L2Devices will have the same deviceNode */
> 
> s/deviceNode/deviceNode.

Done.

> 
>> +	ret = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
> 
> You can assign fd[0] directly instead of going through ret.

Done.

> 
>> +	if (ret < 0) {
>> +		ret = -errno;
>> +		LOG(V4L2, Error)
>> +			<< "Failed to open V4L2 M2M device: " << strerror(-ret);
>> +		return m2m;
> 
> m2m hasn't been initialised at this point. You should probably
> initialise it when declaring the variable. If you do the same for fd
> (initialising them both to -1) you could merge the two error labels
> below.

I've refactored to initialise first by creating the new V4L2VideoDevice
instances as suggested below.

On any error, the instances are deleted, and the pointers set to
nullptr, simplifying the error path.

> 
>> +	}
>> +	fd[0] = ret;
>> +
>> +	ret = dup(fd[0]);
> 
> And you can assign fd[1] here.

Done.

> 
>> +	if (ret < 0) {
>> +		ret = -errno;
>> +		LOG(V4L2, Error)
>> +			<< "Failed to duplicate M2M device: " << strerror(-ret);
>> +
>> +		close(fd[0]);
>> +
>> +		goto err_dup;
> 
> Double close ?

Fixed.

> 
>> +	}
>> +	fd[1] = ret;
>> +
>> +	m2m.output = new V4L2Device(deviceNode);
>> +	m2m.capture = new V4L2Device(deviceNode);
> 
> You could move this to the beginning of the function, avoiding the need
> for initialising m2m to nullptr when declaring it.

Done.

> 
>> +
>> +	ret = m2m.output->setFD(fd[0], V4L2M2MOutput);
>> +	if (ret)
>> +		goto err_device;
>> +
>> +	ret = m2m.capture->setFD(fd[1], V4L2M2MCapture);
>> +	if (ret)
>> +		goto err_device;
>> +
>> +	return m2m;
>> +
>> +err_device:
>> +	delete m2m.capture;
>> +	delete m2m.output;
>> +
>> +	close(fd[1]);
>> +err_dup:
>> +	close(fd[0]);
>> +
>> +	m2m.capture = nullptr;
>> +	m2m.output = nullptr;
>> +
>> +	return m2m;
>> +}
>> +
>>  } /* namespace libcamera */
> 

-- 
Regards
--
Kieran