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

[Laurent Pinchart]

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/ ?

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.

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

> +	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() ?

> +
> +	fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable);
> +	fdEvent_->setEnabled(false);
> +
> +	return 0;
> +}
> +
> +
>  /**
>   * \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.

> +	ret = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);

You can assign fd[0] directly instead of going through ret.

> +	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.

> +	}
> +	fd[0] = ret;
> +
> +	ret = dup(fd[0]);

And you can assign fd[1] here.

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

Double close ?

> +	}
> +	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.

> +
> +	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,

Laurent Pinchart