[libcamera-devel] libcamera: pipeline: Add Simple pipeline

2103 diff mbox series

This add a generic pipeline handler for simple pipelines. It currently
handles pipelines where there's only a PHY node and a sensor node and
the kernel sets up the graph.

The current implementation can deal with sunxi sun6i-csi pipelines but
other simple pipelines can be added to the infos array.

Signed-off-by: Martijn Braam <martijn@brixit.nl>
---
 src/libcamera/pipeline/meson.build |   1 +
 src/libcamera/pipeline/simple.cpp  | 457 +++++++++++++++++++++++++++++
 2 files changed, 458 insertions(+)
 create mode 100644 src/libcamera/pipeline/simple.cpp

Hi Martijn,

Thank you for the patch.

On Fri, Oct 04, 2019 at 09:45:34PM +0200, Martijn Braam wrote:
> This add a generic pipeline handler for simple pipelines. It currently
> handles pipelines where there's only a PHY node and a sensor node and
> the kernel sets up the graph.
> 
> The current implementation can deal with sunxi sun6i-csi pipelines but
> other simple pipelines can be added to the infos array.
> 
> Signed-off-by: Martijn Braam <martijn@brixit.nl>
> ---
>  src/libcamera/pipeline/meson.build |   1 +
>  src/libcamera/pipeline/simple.cpp  | 457 +++++++++++++++++++++++++++++
>  2 files changed, 458 insertions(+)
>  create mode 100644 src/libcamera/pipeline/simple.cpp
> 
> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> index 0d46622..df95a43 100644
> --- a/src/libcamera/pipeline/meson.build
> +++ b/src/libcamera/pipeline/meson.build
> @@ -1,6 +1,7 @@
>  libcamera_sources += files([
>      'uvcvideo.cpp',
>      'vimc.cpp',
> +    'simple.cpp'

Could you please keep the entries alphabetically sorted ? The last entry
should also end with a comma. If the vimc entry hadn't ended with a
comma, your change would have been

 libcamera_sources += files([
     'uvcvideo.cpp',
-    'vimc.cpp'
+    'vimc.cpp',
+    'simple.cpp'
 ])

Ending the last entry with a comma eases addition of new entries.

>  ])
>  
>  subdir('ipu3')
> diff --git a/src/libcamera/pipeline/simple.cpp b/src/libcamera/pipeline/simple.cpp
> new file mode 100644
> index 0000000..f150d7e
> --- /dev/null
> +++ b/src/libcamera/pipeline/simple.cpp
> @@ -0,0 +1,457 @@
> +
> +#include <algorithm>
> +#include <array>
> +#include <iomanip>
> +#include <memory>
> +#include <vector>
> +
> +#include <linux/media-bus-format.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/request.h>
> +#include <libcamera/stream.h>
> +
> +#include "camera_sensor.h"
> +#include "device_enumerator.h"
> +#include "log.h"
> +#include "media_device.h"
> +#include "pipeline_handler.h"
> +#include "utils.h"
> +#include "v4l2_subdevice.h"
> +#include "v4l2_videodevice.h"
> +
> +namespace libcamera {

One blank line here plase.

> +LOG_DEFINE_CATEGORY(Simple)

I would name the log category SimplePipeline, as Simple is quite generic
and would likely be confusing in log messages.

> +
> +struct SimplePipelineInfo {
> +	std::string driverName;
> +	std::string phyName;

Should we use "camera interface" instead of "PHY", as the simple
pipeline handler shouldn't be limited to CSI-2 ? Field names could be
abbreviated to interface or even intf.

> +	std::string v4l2Name;
> +	unsigned int v4l2PixFmt;
> +	unsigned int mediaBusFmt;
> +	unsigned int maxWidth;
> +	unsigned int maxHeight;
> +};
> +
> +class SimpleCameraData : public CameraData
> +{
> +public:
> +	SimpleCameraData(PipelineHandler *pipe)
> +		: CameraData(pipe), sensor_(nullptr)
> +	{
> +	}
> +
> +	~SimpleCameraData()
> +	{
> +		delete sensor_;
> +	}
> +
> +	Stream stream_;
> +	CameraSensor *sensor_;
> +};
> +
> +class SimpleCameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	SimpleCameraConfiguration(Camera *camera, SimpleCameraData *data, const SimplePipelineInfo *pipelineInfo);

Could you please wrap lines to keep them short ? The soft limit is 80
columns and the hard limit 120. While you're below 120, the following
would be more readable.

	SimpleCameraConfiguration(Camera *camera, SimpleCameraData *data,
				  const SimplePipelineInfo *pipelineInfo);

> +
> +	Status validate() override;
> +
> +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +
> +private:
> +	/*
> +         * The SimpleCameraData instance is guaranteed to be valid as long as the
> +         * corresponding Camera instance is valid. In order to borrow a
> +         * reference to the camera data, store a new reference to the camera.
> +         */

Please indent using tabs (here and for other comments below).

> +	std::shared_ptr<Camera> camera_;
> +	const SimpleCameraData *data_;
> +
> +	V4L2SubdeviceFormat sensorFormat_;
> +
> +	const SimplePipelineInfo *pipelineInfo_;

If you want to shorten lines you can rename this to info_. Same for the
identically named field in PipelineHandlerSimple.

> +};
> +
> +class PipelineHandlerSimple : public PipelineHandler
> +{
> +public:
> +	PipelineHandlerSimple(CameraManager *manager);
> +

No need for a blank line betwen the constructor and destructor.

> +	~PipelineHandlerSimple();
> +
> +	CameraConfiguration *generateConfiguration(Camera *camera,
> +						   const StreamRoles &roles) override;
> +
> +	int configure(Camera *camera, CameraConfiguration *config) override;
> +
> +	int allocateBuffers(Camera *camera,
> +			    const std::set<Stream *> &streams) override;
> +
> +	int freeBuffers(Camera *camera,
> +			const std::set<Stream *> &streams) override;
> +
> +	int start(Camera *camera) override;
> +
> +	void stop(Camera *camera) override;
> +
> +	int queueRequest(Camera *camera, Request *request) override;
> +
> +	bool match(DeviceEnumerator *enumerator) override;

You can also remove most of the blank lines to group related functions
together. I recommend using the same style as the base PipelineHandler
class.

> +
> +private:
> +	SimpleCameraData *cameraData(const Camera *camera)
> +	{
> +		return static_cast<SimpleCameraData *>(
> +			PipelineHandler::cameraData(camera));
> +	}
> +
> +	int initLinks();
> +
> +	int createCamera(MediaEntity *sensor);
> +
> +	void bufferReady(Buffer *buffer);
> +
> +	MediaDevice *media_;
> +	V4L2Subdevice *dphy_;
> +	V4L2VideoDevice *video_;
> +
> +	Camera *activeCamera_;
> +
> +	const SimplePipelineInfo *pipelineInfo_;
> +};
> +
> +SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
> +						     SimpleCameraData *data,
> +						     const SimplePipelineInfo *pipelineInfo)
> +	: CameraConfiguration()
> +{
> +	camera_ = camera->shared_from_this();
> +	data_ = data;
> +	pipelineInfo_ = pipelineInfo;
> +}
> +
> +CameraConfiguration::Status SimpleCameraConfiguration::validate()
> +{
> +	const CameraSensor *sensor = data_->sensor_;
> +	Status status = Valid;
> +
> +	if (config_.empty())
> +		return Invalid;
> +
> +	/* Cap the number of entries to the available streams. */
> +	if (config_.size() > 1) {
> +		config_.resize(1);
> +		status = Adjusted;
> +	}
> +
> +	StreamConfiguration &cfg = config_[0];
> +
> +	/* Adjust the pixel format. */
> +	if (cfg.pixelFormat != pipelineInfo_->v4l2PixFmt) {
> +		LOG(Simple, Debug) << "Adjusting pixel format";
> +		cfg.pixelFormat = pipelineInfo_->v4l2PixFmt;
> +		status = Adjusted;
> +	}

This is the first big change that I think is required, you shouldn't
hardcode the v4l2PixFmt, mediaBusFmt or max width/height in
SimplePipelineInfo. They should instead be determined dynamically at
init time through the V4L2 API on the video node and subdevs.

> +
> +	/* Select the sensor format. */
> +	sensorFormat_ = sensor->getFormat({ pipelineInfo_->mediaBusFmt },
> +					  cfg.size);
> +	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> +		sensorFormat_.size = sensor->resolution();
> +
> +	/*
> +         * Provide a suitable default that matches the sensor aspect
> +         * ratio and clamp the size to the hardware bounds.
> +         *
> +         * \todo: Check the hardware alignment constraints.
> +         */
> +	const Size size = cfg.size;
> +
> +	unsigned int pipelineMaxWidth = std::min(sensorFormat_.size.width, pipelineInfo_->maxWidth);
> +	unsigned int pipelineMaxHeight = std::min(sensorFormat_.size.height, pipelineInfo_->maxHeight);
> +
> +	if (!cfg.size.width || !cfg.size.height) {
> +		cfg.size.width = pipelineMaxWidth;
> +		cfg.size.height = pipelineMaxWidth * sensorFormat_.size.height / sensorFormat_.size.width;
> +	}
> +
> +	cfg.size.width = std::min(pipelineMaxWidth, cfg.size.width);
> +	cfg.size.height = std::min(pipelineMaxHeight, cfg.size.height);
> +
> +	cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
> +	cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));
> +
> +	if (cfg.size != size) {
> +		LOG(Simple, Debug)
> +			<< "Adjusting size from " << size.toString()
> +			<< " to " << cfg.size.toString();
> +		status = Adjusted;
> +	}
> +
> +	cfg.bufferCount = 3;
> +
> +	return status;
> +}
> +
> +PipelineHandlerSimple::PipelineHandlerSimple(CameraManager *manager)
> +	: PipelineHandler(manager), dphy_(nullptr), video_(nullptr)
> +{
> +}
> +
> +PipelineHandlerSimple::~PipelineHandlerSimple()
> +{
> +	delete video_;
> +	delete dphy_;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Pipeline Operations
> + */
> +
> +CameraConfiguration *PipelineHandlerSimple::generateConfiguration(Camera *camera,
> +								  const StreamRoles &roles)
> +{
> +	SimpleCameraData *data = cameraData(camera);
> +	CameraConfiguration *config = new SimpleCameraConfiguration(camera, data, PipelineHandlerSimple::pipelineInfo_);

No need to prefix pipelineInfo_ with PipelineHandlerSimple::. The
compiler will resolve symbols based on the context, as this method is a
member of the PipelineHandlerSimple class, it will look for
pipelineInfo_ there.

	CameraConfiguration *config = new SimpleCameraConfiguration(camera, data,
								    pipelineInfo_);

> +
> +	if (roles.empty())
> +		return config;
> +
> +	StreamConfiguration cfg{};
> +	cfg.pixelFormat = pipelineInfo_->v4l2PixFmt;
> +	cfg.size = data->sensor_->resolution();
> +
> +	config->addConfiguration(cfg);
> +
> +	config->validate();
> +
> +	return config;
> +}
> +
> +int PipelineHandlerSimple::configure(Camera *camera, CameraConfiguration *c)
> +{
> +	SimpleCameraConfiguration *config =
> +		static_cast<SimpleCameraConfiguration *>(c);
> +	SimpleCameraData *data = cameraData(camera);
> +	StreamConfiguration &cfg = config->at(0);
> +	CameraSensor *sensor = data->sensor_;
> +	int ret;
> +
> +	/*
> +         * Configure the sensor links: enable the link corresponding to this
> +         * camera and disable all the other sensor links.
> +         */
> +	const MediaPad *pad = dphy_->entity()->getPadByIndex(0);
> +
> +	for (MediaLink *link : pad->links()) {
> +		bool enable = link->source()->entity() == sensor->entity();
> +
> +		if (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)
> +			continue;
> +
> +		LOG(Simple, Debug)
> +			<< (enable ? "Enabling" : "Disabling")
> +			<< " link from sensor '"
> +			<< link->source()->entity()->name()
> +			<< "' to CSI-2 receiver";
> +
> +		ret = link->setEnabled(enable);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/*
> +         * Configure the format on the sensor output and propagate it through
> +         * the pipeline.
> +         */
> +	V4L2SubdeviceFormat format = config->sensorFormat();
> +	LOG(Simple, Debug) << "Configuring sensor with " << format.toString();
> +
> +	ret = sensor->setFormat(&format);
> +	if (ret < 0)
> +		return ret;
> +
> +	LOG(Simple, Debug) << "Sensor configured with " << format.toString();
> +
> +	V4L2DeviceFormat outputFormat = {};
> +	outputFormat.fourcc = cfg.pixelFormat;
> +	outputFormat.size = cfg.size;
> +	outputFormat.planesCount = 2;
> +
> +	ret = video_->setFormat(&outputFormat);
> +	if (ret)
> +		return ret;
> +
> +	if (outputFormat.size != cfg.size ||
> +	    outputFormat.fourcc != cfg.pixelFormat) {
> +		LOG(Simple, Error)
> +			<< "Unable to configure capture in " << cfg.toString();
> +		return -EINVAL;
> +	}
> +
> +	cfg.setStream(&data->stream_);
> +
> +	return 0;
> +}
> +
> +int PipelineHandlerSimple::allocateBuffers(Camera *camera,
> +					   const std::set<Stream *> &streams)
> +{
> +	Stream *stream = *streams.begin();
> +
> +	if (stream->memoryType() == InternalMemory)
> +		return video_->exportBuffers(&stream->bufferPool());
> +	else
> +		return video_->importBuffers(&stream->bufferPool());
> +}
> +
> +int PipelineHandlerSimple::freeBuffers(Camera *camera,
> +				       const std::set<Stream *> &streams)
> +{
> +	if (video_->releaseBuffers())
> +		LOG(Simple, Error) << "Failed to release buffers";
> +
> +	return 0;
> +}
> +
> +int PipelineHandlerSimple::start(Camera *camera)
> +{
> +	int ret;
> +
> +	ret = video_->streamOn();
> +	if (ret)
> +		LOG(Simple, Error)
> +			<< "Failed to start camera " << camera->name();
> +
> +	activeCamera_ = camera;
> +
> +	return ret;

You can return 0 here.

> +}
> +
> +void PipelineHandlerSimple::stop(Camera *camera)
> +{
> +	int ret;
> +
> +	ret = video_->streamOff();
> +	if (ret)
> +		LOG(Simple, Warning)
> +			<< "Failed to stop camera " << camera->name();
> +
> +	activeCamera_ = nullptr;
> +}
> +
> +int PipelineHandlerSimple::queueRequest(Camera *camera, Request *request)
> +{
> +	SimpleCameraData *data = cameraData(camera);
> +	Stream *stream = &data->stream_;
> +
> +	Buffer *buffer = request->findBuffer(stream);
> +	if (!buffer) {
> +		LOG(Simple, Error)
> +			<< "Attempt to queue request with invalid stream";
> +		return -ENOENT;
> +	}
> +
> +	int ret = video_->queueBuffer(buffer);
> +	if (ret < 0)
> +		return ret;
> +
> +	PipelineHandler::queueRequest(camera, request);
> +
> +	return 0;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Match and Setup
> + */
> +
> +int PipelineHandlerSimple::createCamera(MediaEntity *sensor)
> +{
> +	int ret;
> +
> +	std::unique_ptr<SimpleCameraData> data =
> +		utils::make_unique<SimpleCameraData>(this);
> +
> +	data->sensor_ = new CameraSensor(sensor);
> +	ret = data->sensor_->init();
> +	if (ret)
> +		return ret;
> +
> +	std::set<Stream *> streams{ &data->stream_ };
> +	std::shared_ptr<Camera> camera =
> +		Camera::create(this, sensor->name(), streams);
> +	registerCamera(std::move(camera), std::move(data));
> +
> +	return 0;
> +}
> +
> +bool PipelineHandlerSimple::match(DeviceEnumerator *enumerator)
> +{
> +	const MediaPad *pad;
> +
> +	static const SimplePipelineInfo infos[1] = {

No need to specify the array size, the compiler will deduce it from the
initialiser.

> +		{ .driverName = "sun6i-csi",
> +		  .phyName = "sun6i-csi",
> +		  .v4l2Name = "sun6i-csi",
> +		  .v4l2PixFmt = V4L2_PIX_FMT_UYVY,
> +		  .mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,
> +		  .maxWidth = 1280,
> +		  .maxHeight = 720 }

Please add a comma after the last elements, with a new line after the
opening and before the closing curly braces.

		{
			.driverName = "sun6i-csi",
			.phyName = "sun6i-csi",
			.v4l2Name = "sun6i-csi",
			.v4l2PixFmt = V4L2_PIX_FMT_UYVY,
			.mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,
			.maxWidth = 1280,
			.maxHeight = 720,
		},

> +	};
> +
> +	const SimplePipelineInfo *ptr = infos;
> +	for (int i = 0; i < 1; i++, ptr++) {

i is never negative, it should be an unsigned int. You can replace the
hardcoded 1 with ARRAY_SIZE(info). Or even better,

	for (const SimplePipelineInfo *info : infos) {

> +		DeviceMatch dm(ptr->driverName);
> +		dm.add(ptr->phyName);
> +
> +		media_ = acquireMediaDevice(enumerator, dm);
> +		if (!media_)
> +			continue;
> +
> +		PipelineHandlerSimple::pipelineInfo_ = ptr;
> +

As the rest of the loop will only be executed once, how about

		if (media_) {
			pipelineInfo_ = info;
			break;
		}
	}

	if (!media_)
		return false;

and then lowering the indentation of the rest of the code ?

> +		/* Create the V4L2 subdevices we will need. */
> +		dphy_ = V4L2Subdevice::fromEntityName(media_, ptr->phyName);
> +		if (dphy_->open() < 0)
> +			return false;
> +
> +		/* Locate and open the capture video node. */
> +		video_ = V4L2VideoDevice::fromEntityName(media_, ptr->v4l2Name);
> +		if (video_->open() < 0)
> +			return false;
> +
> +		video_->bufferReady.connect(this, &PipelineHandlerSimple::bufferReady);
> +
> +		/*
> +             * Enumerate all sensors connected to the CSI-2 receiver and create one
> +             * camera instance for each of them.
> +             */
> +		pad = dphy_->entity()->getPadByIndex(0);
> +		if (!pad)
> +			return false;

I think we need to make this a bit more generic. I would like to avoid
having to hardcode phyName and v4l2Name in SimplePipelineInfo, and
instead discover the pipeline by looking at entities. In a nutshell, the
code should

- Find the video capture entity. You can do this by looking for an
  entity that reports the MEDIA_ENT_F_IO_V4L function. You can return an
  error if there's more than one.

- Walk the pipeline from entity to entity by going through links to
  locate sensors and the camera interface subdev. You should return an
  error if the pipeline isn't linear (if an entity has more than one
  sink pad or more than one source pad). You shouldn't assume that the
  sink pad will be numbered 0 and the source pad numbered 1, you should
  instead check the pad flags.

For now you can limit support to pipelines with the following topology.

Sensor 1 --\
...         }--> Camera interface subdev -> Video capture device
Sensor n --/

We can later support pipelines than have additional entities.

Thanks a lot for your work, we really appreciate your contribution. I
realise I'm requesting relatively large changes, so please feel free to
request help if needed.

> +
> +		for (MediaLink *link : pad->links())
> +			createCamera(link->source()->entity());
> +
> +		return true;
> +	}
> +	return false;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Buffer Handling
> + */
> +
> +void PipelineHandlerSimple::bufferReady(Buffer *buffer)
> +{
> +	ASSERT(activeCamera_);
> +	LOG(Simple, Debug) << "bufferReady";

I think you can drop the log message here. The V4L2VideoDevice class
already has a debug message before emitting the buffer ready signal, it
should be enough for debugging purpose.

> +	Request *request = buffer->request();
> +	completeBuffer(activeCamera_, request, buffer);
> +	completeRequest(activeCamera_, request);
> +}
> +
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerSimple);
> +
> +} // namespace libcamera

Hi Martijn,

I'm resuming work on this, and I've noticed that the simple.cpp file is
missing the copyright header. Could you please confirm that the source
is covered by LGPL-2.1-or-later, and tell me what copyright notice I
should add (whether to attribute the copyright to you, or your employer)
?

On Sun, Oct 06, 2019 at 05:48:15AM +0300, Laurent Pinchart wrote:
> On Fri, Oct 04, 2019 at 09:45:34PM +0200, Martijn Braam wrote:
> > This add a generic pipeline handler for simple pipelines. It currently
> > handles pipelines where there's only a PHY node and a sensor node and
> > the kernel sets up the graph.
> > 
> > The current implementation can deal with sunxi sun6i-csi pipelines but
> > other simple pipelines can be added to the infos array.
> > 
> > Signed-off-by: Martijn Braam <martijn@brixit.nl>
> > ---
> >  src/libcamera/pipeline/meson.build |   1 +
> >  src/libcamera/pipeline/simple.cpp  | 457 +++++++++++++++++++++++++++++
> >  2 files changed, 458 insertions(+)
> >  create mode 100644 src/libcamera/pipeline/simple.cpp
> > 
> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> > index 0d46622..df95a43 100644
> > --- a/src/libcamera/pipeline/meson.build
> > +++ b/src/libcamera/pipeline/meson.build
> > @@ -1,6 +1,7 @@
> >  libcamera_sources += files([
> >      'uvcvideo.cpp',
> >      'vimc.cpp',
> > +    'simple.cpp'
> 
> Could you please keep the entries alphabetically sorted ? The last entry
> should also end with a comma. If the vimc entry hadn't ended with a
> comma, your change would have been
> 
>  libcamera_sources += files([
>      'uvcvideo.cpp',
> -    'vimc.cpp'
> +    'vimc.cpp',
> +    'simple.cpp'
>  ])
> 
> Ending the last entry with a comma eases addition of new entries.
> 
> >  ])
> >  
> >  subdir('ipu3')
> > diff --git a/src/libcamera/pipeline/simple.cpp b/src/libcamera/pipeline/simple.cpp
> > new file mode 100644
> > index 0000000..f150d7e
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/simple.cpp
> > @@ -0,0 +1,457 @@
> > +
> > +#include <algorithm>
> > +#include <array>
> > +#include <iomanip>
> > +#include <memory>
> > +#include <vector>
> > +
> > +#include <linux/media-bus-format.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/request.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "camera_sensor.h"
> > +#include "device_enumerator.h"
> > +#include "log.h"
> > +#include "media_device.h"
> > +#include "pipeline_handler.h"
> > +#include "utils.h"
> > +#include "v4l2_subdevice.h"
> > +#include "v4l2_videodevice.h"
> > +
> > +namespace libcamera {
> 
> One blank line here plase.
> 
> > +LOG_DEFINE_CATEGORY(Simple)
> 
> I would name the log category SimplePipeline, as Simple is quite generic
> and would likely be confusing in log messages.
> 
> > +
> > +struct SimplePipelineInfo {
> > +	std::string driverName;
> > +	std::string phyName;
> 
> Should we use "camera interface" instead of "PHY", as the simple
> pipeline handler shouldn't be limited to CSI-2 ? Field names could be
> abbreviated to interface or even intf.
> 
> > +	std::string v4l2Name;
> > +	unsigned int v4l2PixFmt;
> > +	unsigned int mediaBusFmt;
> > +	unsigned int maxWidth;
> > +	unsigned int maxHeight;
> > +};
> > +
> > +class SimpleCameraData : public CameraData
> > +{
> > +public:
> > +	SimpleCameraData(PipelineHandler *pipe)
> > +		: CameraData(pipe), sensor_(nullptr)
> > +	{
> > +	}
> > +
> > +	~SimpleCameraData()
> > +	{
> > +		delete sensor_;
> > +	}
> > +
> > +	Stream stream_;
> > +	CameraSensor *sensor_;
> > +};
> > +
> > +class SimpleCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > +	SimpleCameraConfiguration(Camera *camera, SimpleCameraData *data, const SimplePipelineInfo *pipelineInfo);
> 
> Could you please wrap lines to keep them short ? The soft limit is 80
> columns and the hard limit 120. While you're below 120, the following
> would be more readable.
> 
> 	SimpleCameraConfiguration(Camera *camera, SimpleCameraData *data,
> 				  const SimplePipelineInfo *pipelineInfo);
> 
> > +
> > +	Status validate() override;
> > +
> > +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > +
> > +private:
> > +	/*
> > +         * The SimpleCameraData instance is guaranteed to be valid as long as the
> > +         * corresponding Camera instance is valid. In order to borrow a
> > +         * reference to the camera data, store a new reference to the camera.
> > +         */
> 
> Please indent using tabs (here and for other comments below).
> 
> > +	std::shared_ptr<Camera> camera_;
> > +	const SimpleCameraData *data_;
> > +
> > +	V4L2SubdeviceFormat sensorFormat_;
> > +
> > +	const SimplePipelineInfo *pipelineInfo_;
> 
> If you want to shorten lines you can rename this to info_. Same for the
> identically named field in PipelineHandlerSimple.
> 
> > +};
> > +
> > +class PipelineHandlerSimple : public PipelineHandler
> > +{
> > +public:
> > +	PipelineHandlerSimple(CameraManager *manager);
> > +
> 
> No need for a blank line betwen the constructor and destructor.
> 
> > +	~PipelineHandlerSimple();
> > +
> > +	CameraConfiguration *generateConfiguration(Camera *camera,
> > +						   const StreamRoles &roles) override;
> > +
> > +	int configure(Camera *camera, CameraConfiguration *config) override;
> > +
> > +	int allocateBuffers(Camera *camera,
> > +			    const std::set<Stream *> &streams) override;
> > +
> > +	int freeBuffers(Camera *camera,
> > +			const std::set<Stream *> &streams) override;
> > +
> > +	int start(Camera *camera) override;
> > +
> > +	void stop(Camera *camera) override;
> > +
> > +	int queueRequest(Camera *camera, Request *request) override;
> > +
> > +	bool match(DeviceEnumerator *enumerator) override;
> 
> You can also remove most of the blank lines to group related functions
> together. I recommend using the same style as the base PipelineHandler
> class.
> 
> > +
> > +private:
> > +	SimpleCameraData *cameraData(const Camera *camera)
> > +	{
> > +		return static_cast<SimpleCameraData *>(
> > +			PipelineHandler::cameraData(camera));
> > +	}
> > +
> > +	int initLinks();
> > +
> > +	int createCamera(MediaEntity *sensor);
> > +
> > +	void bufferReady(Buffer *buffer);
> > +
> > +	MediaDevice *media_;
> > +	V4L2Subdevice *dphy_;
> > +	V4L2VideoDevice *video_;
> > +
> > +	Camera *activeCamera_;
> > +
> > +	const SimplePipelineInfo *pipelineInfo_;
> > +};
> > +
> > +SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
> > +						     SimpleCameraData *data,
> > +						     const SimplePipelineInfo *pipelineInfo)
> > +	: CameraConfiguration()
> > +{
> > +	camera_ = camera->shared_from_this();
> > +	data_ = data;
> > +	pipelineInfo_ = pipelineInfo;
> > +}
> > +
> > +CameraConfiguration::Status SimpleCameraConfiguration::validate()
> > +{
> > +	const CameraSensor *sensor = data_->sensor_;
> > +	Status status = Valid;
> > +
> > +	if (config_.empty())
> > +		return Invalid;
> > +
> > +	/* Cap the number of entries to the available streams. */
> > +	if (config_.size() > 1) {
> > +		config_.resize(1);
> > +		status = Adjusted;
> > +	}
> > +
> > +	StreamConfiguration &cfg = config_[0];
> > +
> > +	/* Adjust the pixel format. */
> > +	if (cfg.pixelFormat != pipelineInfo_->v4l2PixFmt) {
> > +		LOG(Simple, Debug) << "Adjusting pixel format";
> > +		cfg.pixelFormat = pipelineInfo_->v4l2PixFmt;
> > +		status = Adjusted;
> > +	}
> 
> This is the first big change that I think is required, you shouldn't
> hardcode the v4l2PixFmt, mediaBusFmt or max width/height in
> SimplePipelineInfo. They should instead be determined dynamically at
> init time through the V4L2 API on the video node and subdevs.
> 
> > +
> > +	/* Select the sensor format. */
> > +	sensorFormat_ = sensor->getFormat({ pipelineInfo_->mediaBusFmt },
> > +					  cfg.size);
> > +	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> > +		sensorFormat_.size = sensor->resolution();
> > +
> > +	/*
> > +         * Provide a suitable default that matches the sensor aspect
> > +         * ratio and clamp the size to the hardware bounds.
> > +         *
> > +         * \todo: Check the hardware alignment constraints.
> > +         */
> > +	const Size size = cfg.size;
> > +
> > +	unsigned int pipelineMaxWidth = std::min(sensorFormat_.size.width, pipelineInfo_->maxWidth);
> > +	unsigned int pipelineMaxHeight = std::min(sensorFormat_.size.height, pipelineInfo_->maxHeight);
> > +
> > +	if (!cfg.size.width || !cfg.size.height) {
> > +		cfg.size.width = pipelineMaxWidth;
> > +		cfg.size.height = pipelineMaxWidth * sensorFormat_.size.height / sensorFormat_.size.width;
> > +	}
> > +
> > +	cfg.size.width = std::min(pipelineMaxWidth, cfg.size.width);
> > +	cfg.size.height = std::min(pipelineMaxHeight, cfg.size.height);
> > +
> > +	cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
> > +	cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));
> > +
> > +	if (cfg.size != size) {
> > +		LOG(Simple, Debug)
> > +			<< "Adjusting size from " << size.toString()
> > +			<< " to " << cfg.size.toString();
> > +		status = Adjusted;
> > +	}
> > +
> > +	cfg.bufferCount = 3;
> > +
> > +	return status;
> > +}
> > +
> > +PipelineHandlerSimple::PipelineHandlerSimple(CameraManager *manager)
> > +	: PipelineHandler(manager), dphy_(nullptr), video_(nullptr)
> > +{
> > +}
> > +
> > +PipelineHandlerSimple::~PipelineHandlerSimple()
> > +{
> > +	delete video_;
> > +	delete dphy_;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Pipeline Operations
> > + */
> > +
> > +CameraConfiguration *PipelineHandlerSimple::generateConfiguration(Camera *camera,
> > +								  const StreamRoles &roles)
> > +{
> > +	SimpleCameraData *data = cameraData(camera);
> > +	CameraConfiguration *config = new SimpleCameraConfiguration(camera, data, PipelineHandlerSimple::pipelineInfo_);
> 
> No need to prefix pipelineInfo_ with PipelineHandlerSimple::. The
> compiler will resolve symbols based on the context, as this method is a
> member of the PipelineHandlerSimple class, it will look for
> pipelineInfo_ there.
> 
> 	CameraConfiguration *config = new SimpleCameraConfiguration(camera, data,
> 								    pipelineInfo_);
> 
> > +
> > +	if (roles.empty())
> > +		return config;
> > +
> > +	StreamConfiguration cfg{};
> > +	cfg.pixelFormat = pipelineInfo_->v4l2PixFmt;
> > +	cfg.size = data->sensor_->resolution();
> > +
> > +	config->addConfiguration(cfg);
> > +
> > +	config->validate();
> > +
> > +	return config;
> > +}
> > +
> > +int PipelineHandlerSimple::configure(Camera *camera, CameraConfiguration *c)
> > +{
> > +	SimpleCameraConfiguration *config =
> > +		static_cast<SimpleCameraConfiguration *>(c);
> > +	SimpleCameraData *data = cameraData(camera);
> > +	StreamConfiguration &cfg = config->at(0);
> > +	CameraSensor *sensor = data->sensor_;
> > +	int ret;
> > +
> > +	/*
> > +         * Configure the sensor links: enable the link corresponding to this
> > +         * camera and disable all the other sensor links.
> > +         */
> > +	const MediaPad *pad = dphy_->entity()->getPadByIndex(0);
> > +
> > +	for (MediaLink *link : pad->links()) {
> > +		bool enable = link->source()->entity() == sensor->entity();
> > +
> > +		if (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)
> > +			continue;
> > +
> > +		LOG(Simple, Debug)
> > +			<< (enable ? "Enabling" : "Disabling")
> > +			<< " link from sensor '"
> > +			<< link->source()->entity()->name()
> > +			<< "' to CSI-2 receiver";
> > +
> > +		ret = link->setEnabled(enable);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	/*
> > +         * Configure the format on the sensor output and propagate it through
> > +         * the pipeline.
> > +         */
> > +	V4L2SubdeviceFormat format = config->sensorFormat();
> > +	LOG(Simple, Debug) << "Configuring sensor with " << format.toString();
> > +
> > +	ret = sensor->setFormat(&format);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	LOG(Simple, Debug) << "Sensor configured with " << format.toString();
> > +
> > +	V4L2DeviceFormat outputFormat = {};
> > +	outputFormat.fourcc = cfg.pixelFormat;
> > +	outputFormat.size = cfg.size;
> > +	outputFormat.planesCount = 2;
> > +
> > +	ret = video_->setFormat(&outputFormat);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (outputFormat.size != cfg.size ||
> > +	    outputFormat.fourcc != cfg.pixelFormat) {
> > +		LOG(Simple, Error)
> > +			<< "Unable to configure capture in " << cfg.toString();
> > +		return -EINVAL;
> > +	}
> > +
> > +	cfg.setStream(&data->stream_);
> > +
> > +	return 0;
> > +}
> > +
> > +int PipelineHandlerSimple::allocateBuffers(Camera *camera,
> > +					   const std::set<Stream *> &streams)
> > +{
> > +	Stream *stream = *streams.begin();
> > +
> > +	if (stream->memoryType() == InternalMemory)
> > +		return video_->exportBuffers(&stream->bufferPool());
> > +	else
> > +		return video_->importBuffers(&stream->bufferPool());
> > +}
> > +
> > +int PipelineHandlerSimple::freeBuffers(Camera *camera,
> > +				       const std::set<Stream *> &streams)
> > +{
> > +	if (video_->releaseBuffers())
> > +		LOG(Simple, Error) << "Failed to release buffers";
> > +
> > +	return 0;
> > +}
> > +
> > +int PipelineHandlerSimple::start(Camera *camera)
> > +{
> > +	int ret;
> > +
> > +	ret = video_->streamOn();
> > +	if (ret)
> > +		LOG(Simple, Error)
> > +			<< "Failed to start camera " << camera->name();
> > +
> > +	activeCamera_ = camera;
> > +
> > +	return ret;
> 
> You can return 0 here.
> 
> > +}
> > +
> > +void PipelineHandlerSimple::stop(Camera *camera)
> > +{
> > +	int ret;
> > +
> > +	ret = video_->streamOff();
> > +	if (ret)
> > +		LOG(Simple, Warning)
> > +			<< "Failed to stop camera " << camera->name();
> > +
> > +	activeCamera_ = nullptr;
> > +}
> > +
> > +int PipelineHandlerSimple::queueRequest(Camera *camera, Request *request)
> > +{
> > +	SimpleCameraData *data = cameraData(camera);
> > +	Stream *stream = &data->stream_;
> > +
> > +	Buffer *buffer = request->findBuffer(stream);
> > +	if (!buffer) {
> > +		LOG(Simple, Error)
> > +			<< "Attempt to queue request with invalid stream";
> > +		return -ENOENT;
> > +	}
> > +
> > +	int ret = video_->queueBuffer(buffer);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	PipelineHandler::queueRequest(camera, request);
> > +
> > +	return 0;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Match and Setup
> > + */
> > +
> > +int PipelineHandlerSimple::createCamera(MediaEntity *sensor)
> > +{
> > +	int ret;
> > +
> > +	std::unique_ptr<SimpleCameraData> data =
> > +		utils::make_unique<SimpleCameraData>(this);
> > +
> > +	data->sensor_ = new CameraSensor(sensor);
> > +	ret = data->sensor_->init();
> > +	if (ret)
> > +		return ret;
> > +
> > +	std::set<Stream *> streams{ &data->stream_ };
> > +	std::shared_ptr<Camera> camera =
> > +		Camera::create(this, sensor->name(), streams);
> > +	registerCamera(std::move(camera), std::move(data));
> > +
> > +	return 0;
> > +}
> > +
> > +bool PipelineHandlerSimple::match(DeviceEnumerator *enumerator)
> > +{
> > +	const MediaPad *pad;
> > +
> > +	static const SimplePipelineInfo infos[1] = {
> 
> No need to specify the array size, the compiler will deduce it from the
> initialiser.
> 
> > +		{ .driverName = "sun6i-csi",
> > +		  .phyName = "sun6i-csi",
> > +		  .v4l2Name = "sun6i-csi",
> > +		  .v4l2PixFmt = V4L2_PIX_FMT_UYVY,
> > +		  .mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,
> > +		  .maxWidth = 1280,
> > +		  .maxHeight = 720 }
> 
> Please add a comma after the last elements, with a new line after the
> opening and before the closing curly braces.
> 
> 		{
> 			.driverName = "sun6i-csi",
> 			.phyName = "sun6i-csi",
> 			.v4l2Name = "sun6i-csi",
> 			.v4l2PixFmt = V4L2_PIX_FMT_UYVY,
> 			.mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,
> 			.maxWidth = 1280,
> 			.maxHeight = 720,
> 		},
> 
> > +	};
> > +
> > +	const SimplePipelineInfo *ptr = infos;
> > +	for (int i = 0; i < 1; i++, ptr++) {
> 
> i is never negative, it should be an unsigned int. You can replace the
> hardcoded 1 with ARRAY_SIZE(info). Or even better,
> 
> 	for (const SimplePipelineInfo *info : infos) {
> 
> > +		DeviceMatch dm(ptr->driverName);
> > +		dm.add(ptr->phyName);
> > +
> > +		media_ = acquireMediaDevice(enumerator, dm);
> > +		if (!media_)
> > +			continue;
> > +
> > +		PipelineHandlerSimple::pipelineInfo_ = ptr;
> > +
> 
> As the rest of the loop will only be executed once, how about
> 
> 		if (media_) {
> 			pipelineInfo_ = info;
> 			break;
> 		}
> 	}
> 
> 	if (!media_)
> 		return false;
> 
> and then lowering the indentation of the rest of the code ?
> 
> > +		/* Create the V4L2 subdevices we will need. */
> > +		dphy_ = V4L2Subdevice::fromEntityName(media_, ptr->phyName);
> > +		if (dphy_->open() < 0)
> > +			return false;
> > +
> > +		/* Locate and open the capture video node. */
> > +		video_ = V4L2VideoDevice::fromEntityName(media_, ptr->v4l2Name);
> > +		if (video_->open() < 0)
> > +			return false;
> > +
> > +		video_->bufferReady.connect(this, &PipelineHandlerSimple::bufferReady);
> > +
> > +		/*
> > +             * Enumerate all sensors connected to the CSI-2 receiver and create one
> > +             * camera instance for each of them.
> > +             */
> > +		pad = dphy_->entity()->getPadByIndex(0);
> > +		if (!pad)
> > +			return false;
> 
> I think we need to make this a bit more generic. I would like to avoid
> having to hardcode phyName and v4l2Name in SimplePipelineInfo, and
> instead discover the pipeline by looking at entities. In a nutshell, the
> code should
> 
> - Find the video capture entity. You can do this by looking for an
>   entity that reports the MEDIA_ENT_F_IO_V4L function. You can return an
>   error if there's more than one.
> 
> - Walk the pipeline from entity to entity by going through links to
>   locate sensors and the camera interface subdev. You should return an
>   error if the pipeline isn't linear (if an entity has more than one
>   sink pad or more than one source pad). You shouldn't assume that the
>   sink pad will be numbered 0 and the source pad numbered 1, you should
>   instead check the pad flags.
> 
> For now you can limit support to pipelines with the following topology.
> 
> Sensor 1 --\
> ...         }--> Camera interface subdev -> Video capture device
> Sensor n --/
> 
> We can later support pipelines than have additional entities.
> 
> Thanks a lot for your work, we really appreciate your contribution. I
> realise I'm requesting relatively large changes, so please feel free to
> request help if needed.
> 
> > +
> > +		for (MediaLink *link : pad->links())
> > +			createCamera(link->source()->entity());
> > +
> > +		return true;
> > +	}
> > +	return false;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Buffer Handling
> > + */
> > +
> > +void PipelineHandlerSimple::bufferReady(Buffer *buffer)
> > +{
> > +	ASSERT(activeCamera_);
> > +	LOG(Simple, Debug) << "bufferReady";
> 
> I think you can drop the log message here. The V4L2VideoDevice class
> already has a debug message before emitting the buffer ready signal, it
> should be enough for debugging purpose.
> 
> > +	Request *request = buffer->request();
> > +	completeBuffer(activeCamera_, request, buffer);
> > +	completeRequest(activeCamera_, request);
> > +}
> > +
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerSimple);
> > +
> > +} // namespace libcamera