[libcamera-devel] libcamera: pipeline: Add Qualcomm 8x16 pipeline

1449 diff mbox series

The pipeline handler for the Qualcomm ISP creates one camera instance per
CSI-2 sensor.

It supports two CSI-2 sensors. One connected to CSI-2 Rx msm_csiphy0 sub-device
and the other one connected to CSI-2 Rx msm_csiphy1 sub-device.

Signed-off-by: Mickael Guene <mickael.guene@st.com>
---

 src/libcamera/pipeline/meson.build               |   1 +
 src/libcamera/pipeline/qcom_camss/meson.build    |   3 +
 src/libcamera/pipeline/qcom_camss/qcom_camss.cpp | 543 +++++++++++++++++++++++
 3 files changed, 547 insertions(+)
 create mode 100644 src/libcamera/pipeline/qcom_camss/meson.build
 create mode 100644 src/libcamera/pipeline/qcom_camss/qcom_camss.cpp

Hi Mickael,

On Tue, Jun 18, 2019 at 01:34:04PM +0200, Mickael Guene wrote:
> The pipeline handler for the Qualcomm ISP creates one camera instance per
> CSI-2 sensor.
> 
> It supports two CSI-2 sensors. One connected to CSI-2 Rx msm_csiphy0 sub-device
> and the other one connected to CSI-2 Rx msm_csiphy1 sub-device.

Please see below for some miscellaneous comments. At the top level, I
wonder if we really need a qcom-specific pipeline handler for this. It
looks like the qcom-camss driver only supports the pass-through mode of
the camera ISP, with a linear pipeline that doesn't expose much
configuration options.

Benjamin Gaignard (CC'ed) has submitted a pipeline handler for the
STM32 (see "[PATCH v2] libcamera: pipeline: stm32: add pipeline handler
for stm32"), which also supports linear pipelines only and doesn't have
much dependencies on a particular hardware. We have discussed with him
the option of creating instead a "simple pipeline handler" that would
support a whole range of simple devices, and I think the qcom-camss
would fit in this category.

Is there a chance you could work with Benjamin to merge the two
implementations ?

> Signed-off-by: Mickael Guene <mickael.guene@st.com>
> ---
> 
>  src/libcamera/pipeline/meson.build               |   1 +
>  src/libcamera/pipeline/qcom_camss/meson.build    |   3 +
>  src/libcamera/pipeline/qcom_camss/qcom_camss.cpp | 543 +++++++++++++++++++++++
>  3 files changed, 547 insertions(+)
>  create mode 100644 src/libcamera/pipeline/qcom_camss/meson.build
>  create mode 100644 src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
> 
> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> index 0d46622..f559884 100644
> --- a/src/libcamera/pipeline/meson.build
> +++ b/src/libcamera/pipeline/meson.build
> @@ -5,3 +5,4 @@ libcamera_sources += files([
>  
>  subdir('ipu3')
>  subdir('rkisp1')
> +subdir('qcom_camss')

Please keep the entries alphabetically sorted.

> diff --git a/src/libcamera/pipeline/qcom_camss/meson.build b/src/libcamera/pipeline/qcom_camss/meson.build
> new file mode 100644
> index 0000000..155482f
> --- /dev/null
> +++ b/src/libcamera/pipeline/qcom_camss/meson.build
> @@ -0,0 +1,3 @@
> +libcamera_sources += files([
> +    'qcom_camss.cpp',
> +])
> diff --git a/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp b/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
> new file mode 100644
> index 0000000..6382b57
> --- /dev/null
> +++ b/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
> @@ -0,0 +1,543 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) STMicroelectronics SA 2019
> + *
> + * qcom_camss.cpp - Pipeline handler for Qualcomm ISP found on 8x16 processors.
> + */
> +
> +#include <algorithm>
> +
> +#include <linux/media-bus-format.h>
> +
> +#include <libcamera/camera.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_device.h"
> +#include "v4l2_subdevice.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(QcomCamss)
> +
> +/* pair of supported media code and pixel format */
> +static const std::array<std::array<unsigned int, 2 >, 16> codes {
> +	{
> +		{MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY},
> +		{MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY},
> +		{MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV},
> +		{MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU},
> +		{MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8},
> +		{MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8},
> +		{MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8},
> +		{MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8},
> +		{MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P},
> +		{MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P},
> +		{MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P},
> +		{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P},
> +		{MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P},
> +		{MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12P},
> +		{MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P},
> +		{MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P},
> +	}
> +};
> +
> +static unsigned int mediaCodeFromPixelFormat(unsigned int pixelFormat)
> +{
> +	for (auto code : codes) {
> +		if (code[1] != pixelFormat)
> +			continue;
> +		return code[0];
> +	}
> +
> +	return codes[0][0];
> +}
> +
> +static unsigned int getDefaultPixelFormat(const CameraSensor *sensor)
> +{
> +	const std::vector< unsigned int > sensorCodes = sensor->mbusCodes();
> +
> +	/* return first matching using codes ordering */
> +	for (auto code : codes) {
> +		if (std::any_of(sensorCodes.begin(), sensorCodes.end(),
> +				[&](unsigned int c) { return c == code[0]; })) {
> +			return code[1];
> +		}
> +	}
> +
> +	return codes[0][0];
> +}
> +
> +static bool isPixelFormatSupportedBySensor(const CameraSensor *sensor,
> +					   unsigned int pixelFormat)
> +{
> +	const std::vector< unsigned int > sensorCodes = sensor->mbusCodes();
> +
> +	for (auto code : codes) {
> +		if (code[1] != pixelFormat)
> +			continue;
> +		if (std::any_of(sensorCodes.begin(), sensorCodes.end(),
> +				[&](unsigned int c) { return c == code[0]; }))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +class QcomCamssVideoPipe
> +{
> +public:
> +	QcomCamssVideoPipe() : video_(nullptr)
> +	{
> +	}
> +	~QcomCamssVideoPipe()
> +	{
> +		for (V4L2Subdevice *device : subDevicePipe_)
> +			delete device;
> +	}
> +
> +	bool create_and_link(MediaDevice *media,
> +			     const std::array<std::string, 4> subDevNames,
> +			     const std::string videoName);

Our coding style uses camelCase for all methods.

The subDevNames and videoName variables should be passed as const
references to avoid an unnecessary copy.

> +	int configure(V4L2SubdeviceFormat &format, StreamConfiguration &cfg);
> +
> +	std::vector<V4L2Subdevice *> subDevicePipe_;
> +	V4L2Device *video_;
> +};
> +
> +class QcomCamssCameraData : public CameraData
> +{
> +public:
> +	QcomCamssCameraData(PipelineHandler *pipe,
> +			    QcomCamssVideoPipe *videoPipe)
> +		: CameraData(pipe), sensor_(nullptr), videoPipe_(videoPipe),
> +		  activeCamera_(nullptr)
> +	{
> +	}
> +
> +	~QcomCamssCameraData()
> +	{
> +		delete sensor_;
> +	}
> +	void bufferReady(Buffer *buffer);
> +
> +	Stream stream_;
> +	CameraSensor *sensor_;
> +	QcomCamssVideoPipe *videoPipe_;
> +	Camera *activeCamera_;
> +};
> +
> +class QcomCamssCameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	QcomCamssCameraConfiguration(Camera *camera, QcomCamssCameraData *data);
> +
> +	Status validate() override;
> +
> +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +
> +private:
> +	static constexpr unsigned int QCOMCAMSS_BUFFER_COUNT = 4;
> +
> +	/*
> +	 * The QcomCamssCameraData 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.
> +	 */
> +	std::shared_ptr<Camera> camera_;
> +	const QcomCamssCameraData *data_;
> +
> +	V4L2SubdeviceFormat sensorFormat_;
> +};
> +
> +class PipelineHandlerQcomCamss : public PipelineHandler
> +{
> +public:
> +	PipelineHandlerQcomCamss(CameraManager *manager)
> +		: PipelineHandler(manager)
> +	{
> +	}
> +	~PipelineHandlerQcomCamss()
> +	{
> +	}
> +
> +	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;
> +
> +private:
> +	static constexpr unsigned int QCOMCAMSS_PIPE_NB = 2;
> +
> +	QcomCamssCameraData *cameraData(const Camera *camera)
> +	{
> +		return static_cast<QcomCamssCameraData *>(
> +			PipelineHandler::cameraData(camera));
> +	}
> +
> +	int createCamera(QcomCamssVideoPipe *videoPipe, MediaEntity *sensor);
> +
> +	MediaDevice *media_;
> +	QcomCamssVideoPipe pipe_[QCOMCAMSS_PIPE_NB];
> +};
> +
> +bool QcomCamssVideoPipe::create_and_link(MediaDevice *media,
> +		const std::array<std::string, 4> subDevNames,
> +		const std::string videoName)
> +{
> +	V4L2Subdevice *subDev;
> +	MediaLink *link;
> +
> +	for (std::string subDevName : subDevNames) {

subDevName should be a const reference, there's no need to duplicate it.

> +		subDev = V4L2Subdevice::fromEntityName(media, subDevName);
> +		if (subDev->open() < 0)
> +			return false;
> +		subDevicePipe_.push_back(subDev);
> +	}
> +	video_ = V4L2Device::fromEntityName(media, videoName);
> +	if (video_->open() < 0)
> +		return false;
> +
> +	/* enable links */

Please start all comments with a capital letter and end them with a
period.

> +	for (unsigned int i = 0; i < subDevNames.size() - 1; i++) {
> +		link = media->link(subDevNames[i], 1, subDevNames[i + 1], 0);
> +		if (!link)
> +			return false;
> +		if (link->setEnabled(true) < 0)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +int QcomCamssVideoPipe::configure(V4L2SubdeviceFormat &format,
> +				  StreamConfiguration &cfg)
> +{
> +	int ret;
> +
> +	for (unsigned int i = 0; i < subDevicePipe_.size() - 1; i++) {
> +		V4L2Subdevice *subDev = subDevicePipe_[i];
> +		ret = subDev->setFormat(0, &format);
> +		if (ret < 0) {
> +			LOG(QcomCamss, Debug) << "Failed to set format for subdev => "
> +					      << ret;
> +			return ret;
> +		}
> +		ret = subDev->getFormat(1, &format);
> +		if (ret < 0) {
> +			LOG(QcomCamss, Debug) << "Failed to get format from subdev => "
> +					      << ret;
> +			return ret;
> +		}
> +	}
> +	ret = subDevicePipe_.back()->setFormat(0, &format);
> +	if (ret < 0) {
> +		LOG(QcomCamss, Debug) << "Failed to set format for subdev => "
> +				      << ret;
> +		return ret;
> +	}
> +
> +	V4L2DeviceFormat outputFormat = {};
> +	outputFormat.fourcc = cfg.pixelFormat;
> +	outputFormat.size = cfg.size;
> +	/* our supported formats are single plane only */
> +	outputFormat.planesCount = 1;
> +
> +	ret = video_->setFormat(&outputFormat);
> +	LOG(QcomCamss, Debug) << " video_ setFormat => " << ret;
> +	if (ret) {
> +		LOG(QcomCamss, Debug) << "Failed to set format for video_ => "
> +				      << ret;
> +		return ret;
> +	}
> +
> +	if (outputFormat.size != cfg.size ||
> +	    outputFormat.fourcc != cfg.pixelFormat) {
> +		LOG(QcomCamss, Error) << "Unable to configure capture for "
> +				      << cfg.toString();
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +void QcomCamssCameraData::bufferReady(Buffer *buffer)
> +{
> +	ASSERT(activeCamera_);
> +
> +	Request *request = queuedRequests_.front();
> +
> +	pipe_->completeBuffer(activeCamera_, request, buffer);
> +	pipe_->completeRequest(activeCamera_, request);

The base CameraData class provides you with a pointer to the camera_,
there's no need for an activeCamera_ field.

> +}
> +
> +QcomCamssCameraConfiguration::QcomCamssCameraConfiguration(Camera *camera,
> +		QcomCamssCameraData *data)
> +	: CameraConfiguration()
> +{
> +	camera_ = camera->shared_from_this();
> +	data_ = data;
> +}
> +
> +CameraConfiguration::Status QcomCamssCameraConfiguration::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 needed */
> +	if (!isPixelFormatSupportedBySensor(sensor, cfg.pixelFormat)) {
> +		LOG(QcomCamss, Debug) << "Adjusting format to default one";
> +		cfg.pixelFormat = getDefaultPixelFormat(sensor);
> +		status = Adjusted;
> +	}
> +
> +	/* Select the sensor format. */
> +	sensorFormat_ = sensor->getFormat({ mediaCodeFromPixelFormat(cfg.pixelFormat) },
> +					  cfg.size);
> +	/* test sensorFormat_.mbus_code is valid */
> +	if (!sensorFormat_.mbus_code)
> +		return Invalid;
> +
> +	/* applied to cfg.size */
> +	const Size size = cfg.size;
> +	cfg.size = sensorFormat_.size;
> +	/* clip for hw constraints support */
> +	cfg.size.width = std::max(1U, std::min(8191U, cfg.size.width));
> +	cfg.size.height = std::max(1U, std::min(8191U, cfg.size.height));
> +	if (cfg.size != size) {
> +		LOG(QcomCamss, Debug)
> +			<< "Adjusting size from " << size.toString()
> +			<< " to " << cfg.size.toString();
> +		status = Adjusted;
> +	}
> +
> +	cfg.bufferCount = QCOMCAMSS_BUFFER_COUNT;
> +
> +	return status;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Pipeline Operations
> + */
> +
> +CameraConfiguration *PipelineHandlerQcomCamss::generateConfiguration(
> +	Camera *camera,
> +	const StreamRoles &roles)
> +{
> +	QcomCamssCameraData *data = cameraData(camera);
> +	CameraConfiguration *config =
> +		new QcomCamssCameraConfiguration(camera, data);
> +
> +	if (roles.empty())
> +		return config;
> +
> +	StreamConfiguration cfg{};
> +	cfg.pixelFormat = getDefaultPixelFormat(data->sensor_);
> +	cfg.size = data->sensor_->sizes()[0];
> +
> +	config->addConfiguration(cfg);
> +
> +	config->validate();
> +
> +	return config;
> +}
> +
> +int PipelineHandlerQcomCamss::configure(Camera *camera, CameraConfiguration *c)
> +{
> +	QcomCamssCameraConfiguration *config =
> +		static_cast<QcomCamssCameraConfiguration *>(c);
> +	QcomCamssCameraData *data = cameraData(camera);
> +	StreamConfiguration &cfg = config->at(0);
> +	CameraSensor *sensor = data->sensor_;
> +	QcomCamssVideoPipe *videoPipe = data->videoPipe_;
> +	int ret;
> +
> +	/*
> +	 * Configure the format on the sensor output and propagate it through
> +	 * the pipeline.
> +	 */
> +	V4L2SubdeviceFormat format = config->sensorFormat();
> +	LOG(QcomCamss, Debug) << "Configuring sensor with "
> +			      << format.toString();
> +
> +	ret = sensor->setFormat(&format);
> +	if (ret < 0) {
> +		LOG(QcomCamss, Error) << "Failed to applied format for sensor => "
> +				      << ret;
> +		return ret;
> +	}
> +
> +	ret = videoPipe->configure(format, cfg);
> +	if (ret) {
> +		LOG(QcomCamss, Debug) << "Failed to configure format for video pipe => "
> +				      << ret;
> +		return ret;
> +	}
> +
> +	cfg.setStream(&data->stream_);
> +
> +	return 0;
> +}
> +
> +int PipelineHandlerQcomCamss::allocateBuffers(Camera *camera,
> +					      const std::set<Stream *> &streams)
> +{
> +	QcomCamssCameraData *data = cameraData(camera);
> +
> +	Stream *stream = *streams.begin();
> +
> +	return data->videoPipe_->video_->exportBuffers(&stream->bufferPool());
> +}
> +
> +int PipelineHandlerQcomCamss::freeBuffers(Camera *camera,
> +					  const std::set<Stream *> &streams)
> +{
> +	QcomCamssCameraData *data = cameraData(camera);
> +
> +	if (data->videoPipe_->video_->releaseBuffers())
> +		LOG(QcomCamss, Error) << "Failed to release buffers";
> +
> +	return 0;
> +}
> +
> +int PipelineHandlerQcomCamss::PipelineHandlerQcomCamss::start(Camera *camera)
> +{
> +	QcomCamssCameraData *data = cameraData(camera);
> +	int ret;
> +
> +	ret = data->videoPipe_->video_->streamOn();
> +	if (ret)
> +		LOG(QcomCamss, Error)
> +			<< "Failed to start camera " << camera->name();
> +
> +	data->activeCamera_ = camera;
> +
> +	return ret;
> +}
> +
> +void PipelineHandlerQcomCamss::stop(Camera *camera)
> +{
> +	QcomCamssCameraData *data = cameraData(camera);
> +	int ret;
> +
> +	ret = data->videoPipe_->video_->streamOff();
> +	if (ret)
> +		LOG(QcomCamss, Warning)
> +			<< "Failed to stop camera " << camera->name();
> +
> +	PipelineHandler::stop(camera);
> +
> +	data->activeCamera_ = nullptr;
> +}
> +
> +int PipelineHandlerQcomCamss::queueRequest(Camera *camera, Request *request)
> +{
> +	QcomCamssCameraData *data = cameraData(camera);
> +	Stream *stream = &data->stream_;
> +
> +	Buffer *buffer = request->findBuffer(stream);
> +	if (!buffer) {
> +		LOG(QcomCamss, Error)
> +			<< "Attempt to queue request with invalid stream";
> +		return -ENOENT;
> +	}
> +
> +	int ret = data->videoPipe_->video_->queueBuffer(buffer);
> +	if (ret < 0)
> +		return ret;
> +
> +	PipelineHandler::queueRequest(camera, request);
> +
> +	return 0;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Match and Setup
> + */

Missing blank line.

> +int PipelineHandlerQcomCamss::createCamera(QcomCamssVideoPipe *videoPipe,
> +		MediaEntity *sensor)
> +{
> +	int ret;
> +	std::unique_ptr<QcomCamssCameraData> data =
> +		utils::make_unique<QcomCamssCameraData>(this, videoPipe);
> +
> +	videoPipe->video_->bufferReady.connect(data.get(),
> +			&QcomCamssCameraData::bufferReady);
> +	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 PipelineHandlerQcomCamss::match(DeviceEnumerator *enumerator)
> +{
> +	const std::array<std::array<std::string, 4>, 2> subDevNames = {
> +		{
> +			{"msm_csiphy0", "msm_csid0", "msm_ispif0", "msm_vfe0_rdi0"},
> +			{"msm_csiphy1", "msm_csid1", "msm_ispif1", "msm_vfe0_rdi1"},
> +		}
> +	};
> +	const std::array<std::string, 2> videoNames = {"msm_vfe0_video0", "msm_vfe0_video1"};

These two variables should be static const.

> +	const MediaPad *pad;
> +	DeviceMatch dm("qcom-camss");
> +
> +	/*
> +	 * Code will work with either 8x16 or 8x96 but only expose capabilities
> +	 * of 8x16.
> +	 */
> +	media_ = acquireMediaDevice(enumerator, dm);
> +	if (!media_)
> +		return false;
> +
> +	for (unsigned int i = 0; i < QCOMCAMSS_PIPE_NB; i++) {
> +		if (!pipe_[i].create_and_link(media_, subDevNames[i],
> +					      videoNames[i])) {
> +			LOG(QcomCamss, Error) << "create_and_link failed";
> +			return false;
> +		}
> +
> +		pad = pipe_[i].subDevicePipe_[0]->entity()->getPadByIndex(0);
> +		if (pad)
> +			createCamera(&pipe_[i],
> +				     pad->links()[0]->source()->entity());
> +	}
> +
> +	return true;
> +}
> +
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerQcomCamss);
> +
> +} /* namespace libcamera */
> \ No newline at end of file

Let's add one :-)

Hi Laurent,

 Thanks for the review.
 Find my comments below.

Regards
Mickael

On 6/22/19 23:07, Laurent Pinchart wrote:
> Hi Mickael,
> 
> On Tue, Jun 18, 2019 at 01:34:04PM +0200, Mickael Guene wrote:
>> The pipeline handler for the Qualcomm ISP creates one camera instance per
>> CSI-2 sensor.
>>
>> It supports two CSI-2 sensors. One connected to CSI-2 Rx msm_csiphy0 sub-device
>> and the other one connected to CSI-2 Rx msm_csiphy1 sub-device.
> 
> Please see below for some miscellaneous comments. At the top level, I
> wonder if we really need a qcom-specific pipeline handler for this. It
> looks like the qcom-camss driver only supports the pass-through mode of
> the camera ISP, with a linear pipeline that doesn't expose much
> configuration options.
> 
> Benjamin Gaignard (CC'ed) has submitted a pipeline handler for the
> STM32 (see "[PATCH v2] libcamera: pipeline: stm32: add pipeline handler
> for stm32"), which also supports linear pipelines only and doesn't have
> much dependencies on a particular hardware. We have discussed with him
> the option of creating instead a "simple pipeline handler" that would
> support a whole range of simple devices, and I think the qcom-camss
> would fit in this category.
> 
> Is there a chance you could work with Benjamin to merge the two
> implementations ?
>
 I have talked with Benjamin and we decided not to merge our works.

>> Signed-off-by: Mickael Guene <mickael.guene@st.com>
>> ---
>>
>>  src/libcamera/pipeline/meson.build               |   1 +
>>  src/libcamera/pipeline/qcom_camss/meson.build    |   3 +
>>  src/libcamera/pipeline/qcom_camss/qcom_camss.cpp | 543 +++++++++++++++++++++++
>>  3 files changed, 547 insertions(+)
>>  create mode 100644 src/libcamera/pipeline/qcom_camss/meson.build
>>  create mode 100644 src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
>>
>> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
>> index 0d46622..f559884 100644
>> --- a/src/libcamera/pipeline/meson.build
>> +++ b/src/libcamera/pipeline/meson.build
>> @@ -5,3 +5,4 @@ libcamera_sources += files([
>>  
>>  subdir('ipu3')
>>  subdir('rkisp1')
>> +subdir('qcom_camss')
> 
> Please keep the entries alphabetically sorted.
>
ok

>> diff --git a/src/libcamera/pipeline/qcom_camss/meson.build b/src/libcamera/pipeline/qcom_camss/meson.build
>> new file mode 100644
>> index 0000000..155482f
>> --- /dev/null
>> +++ b/src/libcamera/pipeline/qcom_camss/meson.build
>> @@ -0,0 +1,3 @@
>> +libcamera_sources += files([
>> +    'qcom_camss.cpp',
>> +])
>> diff --git a/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp b/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
>> new file mode 100644
>> index 0000000..6382b57
>> --- /dev/null
>> +++ b/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
>> @@ -0,0 +1,543 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) STMicroelectronics SA 2019
>> + *
>> + * qcom_camss.cpp - Pipeline handler for Qualcomm ISP found on 8x16 processors.
>> + */
>> +
>> +#include <algorithm>
>> +
>> +#include <linux/media-bus-format.h>
>> +
>> +#include <libcamera/camera.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_device.h"
>> +#include "v4l2_subdevice.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(QcomCamss)
>> +
>> +/* pair of supported media code and pixel format */
>> +static const std::array<std::array<unsigned int, 2 >, 16> codes {
>> +	{
>> +		{MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY},
>> +		{MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY},
>> +		{MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV},
>> +		{MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU},
>> +		{MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8},
>> +		{MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8},
>> +		{MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8},
>> +		{MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8},
>> +		{MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P},
>> +		{MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P},
>> +		{MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P},
>> +		{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P},
>> +		{MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P},
>> +		{MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12P},
>> +		{MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P},
>> +		{MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P},
>> +	}
>> +};
>> +
>> +static unsigned int mediaCodeFromPixelFormat(unsigned int pixelFormat)
>> +{
>> +	for (auto code : codes) {
>> +		if (code[1] != pixelFormat)
>> +			continue;
>> +		return code[0];
>> +	}
>> +
>> +	return codes[0][0];
>> +}
>> +
>> +static unsigned int getDefaultPixelFormat(const CameraSensor *sensor)
>> +{
>> +	const std::vector< unsigned int > sensorCodes = sensor->mbusCodes();
>> +
>> +	/* return first matching using codes ordering */
>> +	for (auto code : codes) {
>> +		if (std::any_of(sensorCodes.begin(), sensorCodes.end(),
>> +				[&](unsigned int c) { return c == code[0]; })) {
>> +			return code[1];
>> +		}
>> +	}
>> +
>> +	return codes[0][0];
>> +}
>> +
>> +static bool isPixelFormatSupportedBySensor(const CameraSensor *sensor,
>> +					   unsigned int pixelFormat)
>> +{
>> +	const std::vector< unsigned int > sensorCodes = sensor->mbusCodes();
>> +
>> +	for (auto code : codes) {
>> +		if (code[1] != pixelFormat)
>> +			continue;
>> +		if (std::any_of(sensorCodes.begin(), sensorCodes.end(),
>> +				[&](unsigned int c) { return c == code[0]; }))
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +class QcomCamssVideoPipe
>> +{
>> +public:
>> +	QcomCamssVideoPipe() : video_(nullptr)
>> +	{
>> +	}
>> +	~QcomCamssVideoPipe()
>> +	{
>> +		for (V4L2Subdevice *device : subDevicePipe_)
>> +			delete device;
>> +	}
>> +
>> +	bool create_and_link(MediaDevice *media,
>> +			     const std::array<std::string, 4> subDevNames,
>> +			     const std::string videoName);
> 
> Our coding style uses camelCase for all methods.
>
ok

> The subDevNames and videoName variables should be passed as const
> references to avoid an unnecessary copy.
>
ok
 
>> +	int configure(V4L2SubdeviceFormat &format, StreamConfiguration &cfg);
>> +
>> +	std::vector<V4L2Subdevice *> subDevicePipe_;
>> +	V4L2Device *video_;
>> +};
>> +
>> +class QcomCamssCameraData : public CameraData
>> +{
>> +public:
>> +	QcomCamssCameraData(PipelineHandler *pipe,
>> +			    QcomCamssVideoPipe *videoPipe)
>> +		: CameraData(pipe), sensor_(nullptr), videoPipe_(videoPipe),
>> +		  activeCamera_(nullptr)
>> +	{
>> +	}
>> +
>> +	~QcomCamssCameraData()
>> +	{
>> +		delete sensor_;
>> +	}
>> +	void bufferReady(Buffer *buffer);
>> +
>> +	Stream stream_;
>> +	CameraSensor *sensor_;
>> +	QcomCamssVideoPipe *videoPipe_;
>> +	Camera *activeCamera_;
>> +};
>> +
>> +class QcomCamssCameraConfiguration : public CameraConfiguration
>> +{
>> +public:
>> +	QcomCamssCameraConfiguration(Camera *camera, QcomCamssCameraData *data);
>> +
>> +	Status validate() override;
>> +
>> +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
>> +
>> +private:
>> +	static constexpr unsigned int QCOMCAMSS_BUFFER_COUNT = 4;
>> +
>> +	/*
>> +	 * The QcomCamssCameraData 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.
>> +	 */
>> +	std::shared_ptr<Camera> camera_;
>> +	const QcomCamssCameraData *data_;
>> +
>> +	V4L2SubdeviceFormat sensorFormat_;
>> +};
>> +
>> +class PipelineHandlerQcomCamss : public PipelineHandler
>> +{
>> +public:
>> +	PipelineHandlerQcomCamss(CameraManager *manager)
>> +		: PipelineHandler(manager)
>> +	{
>> +	}
>> +	~PipelineHandlerQcomCamss()
>> +	{
>> +	}
>> +
>> +	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;
>> +
>> +private:
>> +	static constexpr unsigned int QCOMCAMSS_PIPE_NB = 2;
>> +
>> +	QcomCamssCameraData *cameraData(const Camera *camera)
>> +	{
>> +		return static_cast<QcomCamssCameraData *>(
>> +			PipelineHandler::cameraData(camera));
>> +	}
>> +
>> +	int createCamera(QcomCamssVideoPipe *videoPipe, MediaEntity *sensor);
>> +
>> +	MediaDevice *media_;
>> +	QcomCamssVideoPipe pipe_[QCOMCAMSS_PIPE_NB];
>> +};
>> +
>> +bool QcomCamssVideoPipe::create_and_link(MediaDevice *media,
>> +		const std::array<std::string, 4> subDevNames,
>> +		const std::string videoName)
>> +{
>> +	V4L2Subdevice *subDev;
>> +	MediaLink *link;
>> +
>> +	for (std::string subDevName : subDevNames) {
> 
> subDevName should be a const reference, there's no need to duplicate it.
>
ok

>> +		subDev = V4L2Subdevice::fromEntityName(media, subDevName);
>> +		if (subDev->open() < 0)
>> +			return false;
>> +		subDevicePipe_.push_back(subDev);
>> +	}
>> +	video_ = V4L2Device::fromEntityName(media, videoName);
>> +	if (video_->open() < 0)
>> +		return false;
>> +
>> +	/* enable links */
> 
> Please start all comments with a capital letter and end them with a
> period.
>
ok

>> +	for (unsigned int i = 0; i < subDevNames.size() - 1; i++) {
>> +		link = media->link(subDevNames[i], 1, subDevNames[i + 1], 0);
>> +		if (!link)
>> +			return false;
>> +		if (link->setEnabled(true) < 0)
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +int QcomCamssVideoPipe::configure(V4L2SubdeviceFormat &format,
>> +				  StreamConfiguration &cfg)
>> +{
>> +	int ret;
>> +
>> +	for (unsigned int i = 0; i < subDevicePipe_.size() - 1; i++) {
>> +		V4L2Subdevice *subDev = subDevicePipe_[i];
>> +		ret = subDev->setFormat(0, &format);
>> +		if (ret < 0) {
>> +			LOG(QcomCamss, Debug) << "Failed to set format for subdev => "
>> +					      << ret;
>> +			return ret;
>> +		}
>> +		ret = subDev->getFormat(1, &format);
>> +		if (ret < 0) {
>> +			LOG(QcomCamss, Debug) << "Failed to get format from subdev => "
>> +					      << ret;
>> +			return ret;
>> +		}
>> +	}
>> +	ret = subDevicePipe_.back()->setFormat(0, &format);
>> +	if (ret < 0) {
>> +		LOG(QcomCamss, Debug) << "Failed to set format for subdev => "
>> +				      << ret;
>> +		return ret;
>> +	}
>> +
>> +	V4L2DeviceFormat outputFormat = {};
>> +	outputFormat.fourcc = cfg.pixelFormat;
>> +	outputFormat.size = cfg.size;
>> +	/* our supported formats are single plane only */
>> +	outputFormat.planesCount = 1;
>> +
>> +	ret = video_->setFormat(&outputFormat);
>> +	LOG(QcomCamss, Debug) << " video_ setFormat => " << ret;
>> +	if (ret) {
>> +		LOG(QcomCamss, Debug) << "Failed to set format for video_ => "
>> +				      << ret;
>> +		return ret;
>> +	}
>> +
>> +	if (outputFormat.size != cfg.size ||
>> +	    outputFormat.fourcc != cfg.pixelFormat) {
>> +		LOG(QcomCamss, Error) << "Unable to configure capture for "
>> +				      << cfg.toString();
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +void QcomCamssCameraData::bufferReady(Buffer *buffer)
>> +{
>> +	ASSERT(activeCamera_);
>> +
>> +	Request *request = queuedRequests_.front();
>> +
>> +	pipe_->completeBuffer(activeCamera_, request, buffer);
>> +	pipe_->completeRequest(activeCamera_, request);
> 
> The base CameraData class provides you with a pointer to the camera_,
> there's no need for an activeCamera_ field.
>
ok

>> +}
>> +
>> +QcomCamssCameraConfiguration::QcomCamssCameraConfiguration(Camera *camera,
>> +		QcomCamssCameraData *data)
>> +	: CameraConfiguration()
>> +{
>> +	camera_ = camera->shared_from_this();
>> +	data_ = data;
>> +}
>> +
>> +CameraConfiguration::Status QcomCamssCameraConfiguration::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 needed */
>> +	if (!isPixelFormatSupportedBySensor(sensor, cfg.pixelFormat)) {
>> +		LOG(QcomCamss, Debug) << "Adjusting format to default one";
>> +		cfg.pixelFormat = getDefaultPixelFormat(sensor);
>> +		status = Adjusted;
>> +	}
>> +
>> +	/* Select the sensor format. */
>> +	sensorFormat_ = sensor->getFormat({ mediaCodeFromPixelFormat(cfg.pixelFormat) },
>> +					  cfg.size);
>> +	/* test sensorFormat_.mbus_code is valid */
>> +	if (!sensorFormat_.mbus_code)
>> +		return Invalid;
>> +
>> +	/* applied to cfg.size */
>> +	const Size size = cfg.size;
>> +	cfg.size = sensorFormat_.size;
>> +	/* clip for hw constraints support */
>> +	cfg.size.width = std::max(1U, std::min(8191U, cfg.size.width));
>> +	cfg.size.height = std::max(1U, std::min(8191U, cfg.size.height));
>> +	if (cfg.size != size) {
>> +		LOG(QcomCamss, Debug)
>> +			<< "Adjusting size from " << size.toString()
>> +			<< " to " << cfg.size.toString();
>> +		status = Adjusted;
>> +	}
>> +
>> +	cfg.bufferCount = QCOMCAMSS_BUFFER_COUNT;
>> +
>> +	return status;
>> +}
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Pipeline Operations
>> + */
>> +
>> +CameraConfiguration *PipelineHandlerQcomCamss::generateConfiguration(
>> +	Camera *camera,
>> +	const StreamRoles &roles)
>> +{
>> +	QcomCamssCameraData *data = cameraData(camera);
>> +	CameraConfiguration *config =
>> +		new QcomCamssCameraConfiguration(camera, data);
>> +
>> +	if (roles.empty())
>> +		return config;
>> +
>> +	StreamConfiguration cfg{};
>> +	cfg.pixelFormat = getDefaultPixelFormat(data->sensor_);
>> +	cfg.size = data->sensor_->sizes()[0];
>> +
>> +	config->addConfiguration(cfg);
>> +
>> +	config->validate();
>> +
>> +	return config;
>> +}
>> +
>> +int PipelineHandlerQcomCamss::configure(Camera *camera, CameraConfiguration *c)
>> +{
>> +	QcomCamssCameraConfiguration *config =
>> +		static_cast<QcomCamssCameraConfiguration *>(c);
>> +	QcomCamssCameraData *data = cameraData(camera);
>> +	StreamConfiguration &cfg = config->at(0);
>> +	CameraSensor *sensor = data->sensor_;
>> +	QcomCamssVideoPipe *videoPipe = data->videoPipe_;
>> +	int ret;
>> +
>> +	/*
>> +	 * Configure the format on the sensor output and propagate it through
>> +	 * the pipeline.
>> +	 */
>> +	V4L2SubdeviceFormat format = config->sensorFormat();
>> +	LOG(QcomCamss, Debug) << "Configuring sensor with "
>> +			      << format.toString();
>> +
>> +	ret = sensor->setFormat(&format);
>> +	if (ret < 0) {
>> +		LOG(QcomCamss, Error) << "Failed to applied format for sensor => "
>> +				      << ret;
>> +		return ret;
>> +	}
>> +
>> +	ret = videoPipe->configure(format, cfg);
>> +	if (ret) {
>> +		LOG(QcomCamss, Debug) << "Failed to configure format for video pipe => "
>> +				      << ret;
>> +		return ret;
>> +	}
>> +
>> +	cfg.setStream(&data->stream_);
>> +
>> +	return 0;
>> +}
>> +
>> +int PipelineHandlerQcomCamss::allocateBuffers(Camera *camera,
>> +					      const std::set<Stream *> &streams)
>> +{
>> +	QcomCamssCameraData *data = cameraData(camera);
>> +
>> +	Stream *stream = *streams.begin();
>> +
>> +	return data->videoPipe_->video_->exportBuffers(&stream->bufferPool());
>> +}
>> +
>> +int PipelineHandlerQcomCamss::freeBuffers(Camera *camera,
>> +					  const std::set<Stream *> &streams)
>> +{
>> +	QcomCamssCameraData *data = cameraData(camera);
>> +
>> +	if (data->videoPipe_->video_->releaseBuffers())
>> +		LOG(QcomCamss, Error) << "Failed to release buffers";
>> +
>> +	return 0;
>> +}
>> +
>> +int PipelineHandlerQcomCamss::PipelineHandlerQcomCamss::start(Camera *camera)
>> +{
>> +	QcomCamssCameraData *data = cameraData(camera);
>> +	int ret;
>> +
>> +	ret = data->videoPipe_->video_->streamOn();
>> +	if (ret)
>> +		LOG(QcomCamss, Error)
>> +			<< "Failed to start camera " << camera->name();
>> +
>> +	data->activeCamera_ = camera;
>> +
>> +	return ret;
>> +}
>> +
>> +void PipelineHandlerQcomCamss::stop(Camera *camera)
>> +{
>> +	QcomCamssCameraData *data = cameraData(camera);
>> +	int ret;
>> +
>> +	ret = data->videoPipe_->video_->streamOff();
>> +	if (ret)
>> +		LOG(QcomCamss, Warning)
>> +			<< "Failed to stop camera " << camera->name();
>> +
>> +	PipelineHandler::stop(camera);
>> +
>> +	data->activeCamera_ = nullptr;
>> +}
>> +
>> +int PipelineHandlerQcomCamss::queueRequest(Camera *camera, Request *request)
>> +{
>> +	QcomCamssCameraData *data = cameraData(camera);
>> +	Stream *stream = &data->stream_;
>> +
>> +	Buffer *buffer = request->findBuffer(stream);
>> +	if (!buffer) {
>> +		LOG(QcomCamss, Error)
>> +			<< "Attempt to queue request with invalid stream";
>> +		return -ENOENT;
>> +	}
>> +
>> +	int ret = data->videoPipe_->video_->queueBuffer(buffer);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	PipelineHandler::queueRequest(camera, request);
>> +
>> +	return 0;
>> +}
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Match and Setup
>> + */
> 
> Missing blank line.
> 
ok

>> +int PipelineHandlerQcomCamss::createCamera(QcomCamssVideoPipe *videoPipe,
>> +		MediaEntity *sensor)
>> +{
>> +	int ret;
>> +	std::unique_ptr<QcomCamssCameraData> data =
>> +		utils::make_unique<QcomCamssCameraData>(this, videoPipe);
>> +
>> +	videoPipe->video_->bufferReady.connect(data.get(),
>> +			&QcomCamssCameraData::bufferReady);
>> +	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 PipelineHandlerQcomCamss::match(DeviceEnumerator *enumerator)
>> +{
>> +	const std::array<std::array<std::string, 4>, 2> subDevNames = {
>> +		{
>> +			{"msm_csiphy0", "msm_csid0", "msm_ispif0", "msm_vfe0_rdi0"},
>> +			{"msm_csiphy1", "msm_csid1", "msm_ispif1", "msm_vfe0_rdi1"},
>> +		}
>> +	};
>> +	const std::array<std::string, 2> videoNames = {"msm_vfe0_video0", "msm_vfe0_video1"};
> 
> These two variables should be static const.
> 
ok

>> +	const MediaPad *pad;
>> +	DeviceMatch dm("qcom-camss");
>> +
>> +	/*
>> +	 * Code will work with either 8x16 or 8x96 but only expose capabilities
>> +	 * of 8x16.
>> +	 */
>> +	media_ = acquireMediaDevice(enumerator, dm);
>> +	if (!media_)
>> +		return false;
>> +
>> +	for (unsigned int i = 0; i < QCOMCAMSS_PIPE_NB; i++) {
>> +		if (!pipe_[i].create_and_link(media_, subDevNames[i],
>> +					      videoNames[i])) {
>> +			LOG(QcomCamss, Error) << "create_and_link failed";
>> +			return false;
>> +		}
>> +
>> +		pad = pipe_[i].subDevicePipe_[0]->entity()->getPadByIndex(0);
>> +		if (pad)
>> +			createCamera(&pipe_[i],
>> +				     pad->links()[0]->source()->entity());
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +REGISTER_PIPELINE_HANDLER(PipelineHandlerQcomCamss);
>> +
>> +} /* namespace libcamera */
>> \ No newline at end of file
> 
> Let's add one :-)
> 
ok

Hi Mickael,

On Wed, Jun 26, 2019 at 08:34:29AM +0000, Mickael GUENE wrote:
> Hi Laurent,
> 
>  Thanks for the review.
>  Find my comments below.
> 
> Regards
> Mickael
> 
> On 6/22/19 23:07, Laurent Pinchart wrote:
> > Hi Mickael,
> > 
> > On Tue, Jun 18, 2019 at 01:34:04PM +0200, Mickael Guene wrote:
> >> The pipeline handler for the Qualcomm ISP creates one camera instance per
> >> CSI-2 sensor.
> >>
> >> It supports two CSI-2 sensors. One connected to CSI-2 Rx msm_csiphy0 sub-device
> >> and the other one connected to CSI-2 Rx msm_csiphy1 sub-device.
> > 
> > Please see below for some miscellaneous comments. At the top level, I
> > wonder if we really need a qcom-specific pipeline handler for this. It
> > looks like the qcom-camss driver only supports the pass-through mode of
> > the camera ISP, with a linear pipeline that doesn't expose much
> > configuration options.
> > 
> > Benjamin Gaignard (CC'ed) has submitted a pipeline handler for the
> > STM32 (see "[PATCH v2] libcamera: pipeline: stm32: add pipeline handler
> > for stm32"), which also supports linear pipelines only and doesn't have
> > much dependencies on a particular hardware. We have discussed with him
> > the option of creating instead a "simple pipeline handler" that would
> > support a whole range of simple devices, and I think the qcom-camss
> > would fit in this category.
> > 
> > Is there a chance you could work with Benjamin to merge the two
> > implementations ?
>
>  I have talked with Benjamin and we decided not to merge our works.

No worries, but I'm afraid that a qcom-camss-specific pipeline handler
doesn't make much sense without ISP support. It came to my attention
yesterday that work was ongoing on a pipeline handler for the i.MX SoCs,
which could fall in the same category. We don't want to see a
multiplication of nearly identical pipeline handlers for linear
pipelines without an ISP, they should all be supported through the same
code.

> >> Signed-off-by: Mickael Guene <mickael.guene@st.com>
> >> ---
> >>
> >>  src/libcamera/pipeline/meson.build               |   1 +
> >>  src/libcamera/pipeline/qcom_camss/meson.build    |   3 +
> >>  src/libcamera/pipeline/qcom_camss/qcom_camss.cpp | 543 +++++++++++++++++++++++
> >>  3 files changed, 547 insertions(+)
> >>  create mode 100644 src/libcamera/pipeline/qcom_camss/meson.build
> >>  create mode 100644 src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
> >>
> >> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> >> index 0d46622..f559884 100644
> >> --- a/src/libcamera/pipeline/meson.build
> >> +++ b/src/libcamera/pipeline/meson.build
> >> @@ -5,3 +5,4 @@ libcamera_sources += files([
> >>  
> >>  subdir('ipu3')
> >>  subdir('rkisp1')
> >> +subdir('qcom_camss')
> > 
> > Please keep the entries alphabetically sorted.
> >
> ok
> 
> >> diff --git a/src/libcamera/pipeline/qcom_camss/meson.build b/src/libcamera/pipeline/qcom_camss/meson.build
> >> new file mode 100644
> >> index 0000000..155482f
> >> --- /dev/null
> >> +++ b/src/libcamera/pipeline/qcom_camss/meson.build
> >> @@ -0,0 +1,3 @@
> >> +libcamera_sources += files([
> >> +    'qcom_camss.cpp',
> >> +])
> >> diff --git a/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp b/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
> >> new file mode 100644
> >> index 0000000..6382b57
> >> --- /dev/null
> >> +++ b/src/libcamera/pipeline/qcom_camss/qcom_camss.cpp
> >> @@ -0,0 +1,543 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) STMicroelectronics SA 2019
> >> + *
> >> + * qcom_camss.cpp - Pipeline handler for Qualcomm ISP found on 8x16 processors.
> >> + */
> >> +
> >> +#include <algorithm>
> >> +
> >> +#include <linux/media-bus-format.h>
> >> +
> >> +#include <libcamera/camera.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_device.h"
> >> +#include "v4l2_subdevice.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(QcomCamss)
> >> +
> >> +/* pair of supported media code and pixel format */
> >> +static const std::array<std::array<unsigned int, 2 >, 16> codes {
> >> +	{
> >> +		{MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY},
> >> +		{MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY},
> >> +		{MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV},
> >> +		{MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU},
> >> +		{MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8},
> >> +		{MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8},
> >> +		{MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8},
> >> +		{MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8},
> >> +		{MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P},
> >> +		{MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P},
> >> +		{MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P},
> >> +		{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P},
> >> +		{MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P},
> >> +		{MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12P},
> >> +		{MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P},
> >> +		{MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P},
> >> +	}
> >> +};
> >> +
> >> +static unsigned int mediaCodeFromPixelFormat(unsigned int pixelFormat)
> >> +{
> >> +	for (auto code : codes) {
> >> +		if (code[1] != pixelFormat)
> >> +			continue;
> >> +		return code[0];
> >> +	}
> >> +
> >> +	return codes[0][0];
> >> +}
> >> +
> >> +static unsigned int getDefaultPixelFormat(const CameraSensor *sensor)
> >> +{
> >> +	const std::vector< unsigned int > sensorCodes = sensor->mbusCodes();
> >> +
> >> +	/* return first matching using codes ordering */
> >> +	for (auto code : codes) {
> >> +		if (std::any_of(sensorCodes.begin(), sensorCodes.end(),
> >> +				[&](unsigned int c) { return c == code[0]; })) {
> >> +			return code[1];
> >> +		}
> >> +	}
> >> +
> >> +	return codes[0][0];
> >> +}
> >> +
> >> +static bool isPixelFormatSupportedBySensor(const CameraSensor *sensor,
> >> +					   unsigned int pixelFormat)
> >> +{
> >> +	const std::vector< unsigned int > sensorCodes = sensor->mbusCodes();
> >> +
> >> +	for (auto code : codes) {
> >> +		if (code[1] != pixelFormat)
> >> +			continue;
> >> +		if (std::any_of(sensorCodes.begin(), sensorCodes.end(),
> >> +				[&](unsigned int c) { return c == code[0]; }))
> >> +			return true;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >> +class QcomCamssVideoPipe
> >> +{
> >> +public:
> >> +	QcomCamssVideoPipe() : video_(nullptr)
> >> +	{
> >> +	}
> >> +	~QcomCamssVideoPipe()
> >> +	{
> >> +		for (V4L2Subdevice *device : subDevicePipe_)
> >> +			delete device;
> >> +	}
> >> +
> >> +	bool create_and_link(MediaDevice *media,
> >> +			     const std::array<std::string, 4> subDevNames,
> >> +			     const std::string videoName);
> > 
> > Our coding style uses camelCase for all methods.
> >
> ok
> 
> > The subDevNames and videoName variables should be passed as const
> > references to avoid an unnecessary copy.
> >
> ok
>  
> >> +	int configure(V4L2SubdeviceFormat &format, StreamConfiguration &cfg);
> >> +
> >> +	std::vector<V4L2Subdevice *> subDevicePipe_;
> >> +	V4L2Device *video_;
> >> +};
> >> +
> >> +class QcomCamssCameraData : public CameraData
> >> +{
> >> +public:
> >> +	QcomCamssCameraData(PipelineHandler *pipe,
> >> +			    QcomCamssVideoPipe *videoPipe)
> >> +		: CameraData(pipe), sensor_(nullptr), videoPipe_(videoPipe),
> >> +		  activeCamera_(nullptr)
> >> +	{
> >> +	}
> >> +
> >> +	~QcomCamssCameraData()
> >> +	{
> >> +		delete sensor_;
> >> +	}
> >> +	void bufferReady(Buffer *buffer);
> >> +
> >> +	Stream stream_;
> >> +	CameraSensor *sensor_;
> >> +	QcomCamssVideoPipe *videoPipe_;
> >> +	Camera *activeCamera_;
> >> +};
> >> +
> >> +class QcomCamssCameraConfiguration : public CameraConfiguration
> >> +{
> >> +public:
> >> +	QcomCamssCameraConfiguration(Camera *camera, QcomCamssCameraData *data);
> >> +
> >> +	Status validate() override;
> >> +
> >> +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> >> +
> >> +private:
> >> +	static constexpr unsigned int QCOMCAMSS_BUFFER_COUNT = 4;
> >> +
> >> +	/*
> >> +	 * The QcomCamssCameraData 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.
> >> +	 */
> >> +	std::shared_ptr<Camera> camera_;
> >> +	const QcomCamssCameraData *data_;
> >> +
> >> +	V4L2SubdeviceFormat sensorFormat_;
> >> +};
> >> +
> >> +class PipelineHandlerQcomCamss : public PipelineHandler
> >> +{
> >> +public:
> >> +	PipelineHandlerQcomCamss(CameraManager *manager)
> >> +		: PipelineHandler(manager)
> >> +	{
> >> +	}
> >> +	~PipelineHandlerQcomCamss()
> >> +	{
> >> +	}
> >> +
> >> +	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;
> >> +
> >> +private:
> >> +	static constexpr unsigned int QCOMCAMSS_PIPE_NB = 2;
> >> +
> >> +	QcomCamssCameraData *cameraData(const Camera *camera)
> >> +	{
> >> +		return static_cast<QcomCamssCameraData *>(
> >> +			PipelineHandler::cameraData(camera));
> >> +	}
> >> +
> >> +	int createCamera(QcomCamssVideoPipe *videoPipe, MediaEntity *sensor);
> >> +
> >> +	MediaDevice *media_;
> >> +	QcomCamssVideoPipe pipe_[QCOMCAMSS_PIPE_NB];
> >> +};
> >> +
> >> +bool QcomCamssVideoPipe::create_and_link(MediaDevice *media,
> >> +		const std::array<std::string, 4> subDevNames,
> >> +		const std::string videoName)
> >> +{
> >> +	V4L2Subdevice *subDev;
> >> +	MediaLink *link;
> >> +
> >> +	for (std::string subDevName : subDevNames) {
> > 
> > subDevName should be a const reference, there's no need to duplicate it.
> >
> ok
> 
> >> +		subDev = V4L2Subdevice::fromEntityName(media, subDevName);
> >> +		if (subDev->open() < 0)
> >> +			return false;
> >> +		subDevicePipe_.push_back(subDev);
> >> +	}
> >> +	video_ = V4L2Device::fromEntityName(media, videoName);
> >> +	if (video_->open() < 0)
> >> +		return false;
> >> +
> >> +	/* enable links */
> > 
> > Please start all comments with a capital letter and end them with a
> > period.
> >
> ok
> 
> >> +	for (unsigned int i = 0; i < subDevNames.size() - 1; i++) {
> >> +		link = media->link(subDevNames[i], 1, subDevNames[i + 1], 0);
> >> +		if (!link)
> >> +			return false;
> >> +		if (link->setEnabled(true) < 0)
> >> +			return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >> +int QcomCamssVideoPipe::configure(V4L2SubdeviceFormat &format,
> >> +				  StreamConfiguration &cfg)
> >> +{
> >> +	int ret;
> >> +
> >> +	for (unsigned int i = 0; i < subDevicePipe_.size() - 1; i++) {
> >> +		V4L2Subdevice *subDev = subDevicePipe_[i];
> >> +		ret = subDev->setFormat(0, &format);
> >> +		if (ret < 0) {
> >> +			LOG(QcomCamss, Debug) << "Failed to set format for subdev => "
> >> +					      << ret;
> >> +			return ret;
> >> +		}
> >> +		ret = subDev->getFormat(1, &format);
> >> +		if (ret < 0) {
> >> +			LOG(QcomCamss, Debug) << "Failed to get format from subdev => "
> >> +					      << ret;
> >> +			return ret;
> >> +		}
> >> +	}
> >> +	ret = subDevicePipe_.back()->setFormat(0, &format);
> >> +	if (ret < 0) {
> >> +		LOG(QcomCamss, Debug) << "Failed to set format for subdev => "
> >> +				      << ret;
> >> +		return ret;
> >> +	}
> >> +
> >> +	V4L2DeviceFormat outputFormat = {};
> >> +	outputFormat.fourcc = cfg.pixelFormat;
> >> +	outputFormat.size = cfg.size;
> >> +	/* our supported formats are single plane only */
> >> +	outputFormat.planesCount = 1;
> >> +
> >> +	ret = video_->setFormat(&outputFormat);
> >> +	LOG(QcomCamss, Debug) << " video_ setFormat => " << ret;
> >> +	if (ret) {
> >> +		LOG(QcomCamss, Debug) << "Failed to set format for video_ => "
> >> +				      << ret;
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (outputFormat.size != cfg.size ||
> >> +	    outputFormat.fourcc != cfg.pixelFormat) {
> >> +		LOG(QcomCamss, Error) << "Unable to configure capture for "
> >> +				      << cfg.toString();
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +void QcomCamssCameraData::bufferReady(Buffer *buffer)
> >> +{
> >> +	ASSERT(activeCamera_);
> >> +
> >> +	Request *request = queuedRequests_.front();
> >> +
> >> +	pipe_->completeBuffer(activeCamera_, request, buffer);
> >> +	pipe_->completeRequest(activeCamera_, request);
> > 
> > The base CameraData class provides you with a pointer to the camera_,
> > there's no need for an activeCamera_ field.
> >
> ok
> 
> >> +}
> >> +
> >> +QcomCamssCameraConfiguration::QcomCamssCameraConfiguration(Camera *camera,
> >> +		QcomCamssCameraData *data)
> >> +	: CameraConfiguration()
> >> +{
> >> +	camera_ = camera->shared_from_this();
> >> +	data_ = data;
> >> +}
> >> +
> >> +CameraConfiguration::Status QcomCamssCameraConfiguration::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 needed */
> >> +	if (!isPixelFormatSupportedBySensor(sensor, cfg.pixelFormat)) {
> >> +		LOG(QcomCamss, Debug) << "Adjusting format to default one";
> >> +		cfg.pixelFormat = getDefaultPixelFormat(sensor);
> >> +		status = Adjusted;
> >> +	}
> >> +
> >> +	/* Select the sensor format. */
> >> +	sensorFormat_ = sensor->getFormat({ mediaCodeFromPixelFormat(cfg.pixelFormat) },
> >> +					  cfg.size);
> >> +	/* test sensorFormat_.mbus_code is valid */
> >> +	if (!sensorFormat_.mbus_code)
> >> +		return Invalid;
> >> +
> >> +	/* applied to cfg.size */
> >> +	const Size size = cfg.size;
> >> +	cfg.size = sensorFormat_.size;
> >> +	/* clip for hw constraints support */
> >> +	cfg.size.width = std::max(1U, std::min(8191U, cfg.size.width));
> >> +	cfg.size.height = std::max(1U, std::min(8191U, cfg.size.height));
> >> +	if (cfg.size != size) {
> >> +		LOG(QcomCamss, Debug)
> >> +			<< "Adjusting size from " << size.toString()
> >> +			<< " to " << cfg.size.toString();
> >> +		status = Adjusted;
> >> +	}
> >> +
> >> +	cfg.bufferCount = QCOMCAMSS_BUFFER_COUNT;
> >> +
> >> +	return status;
> >> +}
> >> +
> >> +/* -----------------------------------------------------------------------------
> >> + * Pipeline Operations
> >> + */
> >> +
> >> +CameraConfiguration *PipelineHandlerQcomCamss::generateConfiguration(
> >> +	Camera *camera,
> >> +	const StreamRoles &roles)
> >> +{
> >> +	QcomCamssCameraData *data = cameraData(camera);
> >> +	CameraConfiguration *config =
> >> +		new QcomCamssCameraConfiguration(camera, data);
> >> +
> >> +	if (roles.empty())
> >> +		return config;
> >> +
> >> +	StreamConfiguration cfg{};
> >> +	cfg.pixelFormat = getDefaultPixelFormat(data->sensor_);
> >> +	cfg.size = data->sensor_->sizes()[0];
> >> +
> >> +	config->addConfiguration(cfg);
> >> +
> >> +	config->validate();
> >> +
> >> +	return config;
> >> +}
> >> +
> >> +int PipelineHandlerQcomCamss::configure(Camera *camera, CameraConfiguration *c)
> >> +{
> >> +	QcomCamssCameraConfiguration *config =
> >> +		static_cast<QcomCamssCameraConfiguration *>(c);
> >> +	QcomCamssCameraData *data = cameraData(camera);
> >> +	StreamConfiguration &cfg = config->at(0);
> >> +	CameraSensor *sensor = data->sensor_;
> >> +	QcomCamssVideoPipe *videoPipe = data->videoPipe_;
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * Configure the format on the sensor output and propagate it through
> >> +	 * the pipeline.
> >> +	 */
> >> +	V4L2SubdeviceFormat format = config->sensorFormat();
> >> +	LOG(QcomCamss, Debug) << "Configuring sensor with "
> >> +			      << format.toString();
> >> +
> >> +	ret = sensor->setFormat(&format);
> >> +	if (ret < 0) {
> >> +		LOG(QcomCamss, Error) << "Failed to applied format for sensor => "
> >> +				      << ret;
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = videoPipe->configure(format, cfg);
> >> +	if (ret) {
> >> +		LOG(QcomCamss, Debug) << "Failed to configure format for video pipe => "
> >> +				      << ret;
> >> +		return ret;
> >> +	}
> >> +
> >> +	cfg.setStream(&data->stream_);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int PipelineHandlerQcomCamss::allocateBuffers(Camera *camera,
> >> +					      const std::set<Stream *> &streams)
> >> +{
> >> +	QcomCamssCameraData *data = cameraData(camera);
> >> +
> >> +	Stream *stream = *streams.begin();
> >> +
> >> +	return data->videoPipe_->video_->exportBuffers(&stream->bufferPool());
> >> +}
> >> +
> >> +int PipelineHandlerQcomCamss::freeBuffers(Camera *camera,
> >> +					  const std::set<Stream *> &streams)
> >> +{
> >> +	QcomCamssCameraData *data = cameraData(camera);
> >> +
> >> +	if (data->videoPipe_->video_->releaseBuffers())
> >> +		LOG(QcomCamss, Error) << "Failed to release buffers";
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int PipelineHandlerQcomCamss::PipelineHandlerQcomCamss::start(Camera *camera)
> >> +{
> >> +	QcomCamssCameraData *data = cameraData(camera);
> >> +	int ret;
> >> +
> >> +	ret = data->videoPipe_->video_->streamOn();
> >> +	if (ret)
> >> +		LOG(QcomCamss, Error)
> >> +			<< "Failed to start camera " << camera->name();
> >> +
> >> +	data->activeCamera_ = camera;
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +void PipelineHandlerQcomCamss::stop(Camera *camera)
> >> +{
> >> +	QcomCamssCameraData *data = cameraData(camera);
> >> +	int ret;
> >> +
> >> +	ret = data->videoPipe_->video_->streamOff();
> >> +	if (ret)
> >> +		LOG(QcomCamss, Warning)
> >> +			<< "Failed to stop camera " << camera->name();
> >> +
> >> +	PipelineHandler::stop(camera);
> >> +
> >> +	data->activeCamera_ = nullptr;
> >> +}
> >> +
> >> +int PipelineHandlerQcomCamss::queueRequest(Camera *camera, Request *request)
> >> +{
> >> +	QcomCamssCameraData *data = cameraData(camera);
> >> +	Stream *stream = &data->stream_;
> >> +
> >> +	Buffer *buffer = request->findBuffer(stream);
> >> +	if (!buffer) {
> >> +		LOG(QcomCamss, Error)
> >> +			<< "Attempt to queue request with invalid stream";
> >> +		return -ENOENT;
> >> +	}
> >> +
> >> +	int ret = data->videoPipe_->video_->queueBuffer(buffer);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	PipelineHandler::queueRequest(camera, request);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/* -----------------------------------------------------------------------------
> >> + * Match and Setup
> >> + */
> > 
> > Missing blank line.
> > 
> ok
> 
> >> +int PipelineHandlerQcomCamss::createCamera(QcomCamssVideoPipe *videoPipe,
> >> +		MediaEntity *sensor)
> >> +{
> >> +	int ret;
> >> +	std::unique_ptr<QcomCamssCameraData> data =
> >> +		utils::make_unique<QcomCamssCameraData>(this, videoPipe);
> >> +
> >> +	videoPipe->video_->bufferReady.connect(data.get(),
> >> +			&QcomCamssCameraData::bufferReady);
> >> +	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 PipelineHandlerQcomCamss::match(DeviceEnumerator *enumerator)
> >> +{
> >> +	const std::array<std::array<std::string, 4>, 2> subDevNames = {
> >> +		{
> >> +			{"msm_csiphy0", "msm_csid0", "msm_ispif0", "msm_vfe0_rdi0"},
> >> +			{"msm_csiphy1", "msm_csid1", "msm_ispif1", "msm_vfe0_rdi1"},
> >> +		}
> >> +	};
> >> +	const std::array<std::string, 2> videoNames = {"msm_vfe0_video0", "msm_vfe0_video1"};
> > 
> > These two variables should be static const.
> > 
> ok
> 
> >> +	const MediaPad *pad;
> >> +	DeviceMatch dm("qcom-camss");
> >> +
> >> +	/*
> >> +	 * Code will work with either 8x16 or 8x96 but only expose capabilities
> >> +	 * of 8x16.
> >> +	 */
> >> +	media_ = acquireMediaDevice(enumerator, dm);
> >> +	if (!media_)
> >> +		return false;
> >> +
> >> +	for (unsigned int i = 0; i < QCOMCAMSS_PIPE_NB; i++) {
> >> +		if (!pipe_[i].create_and_link(media_, subDevNames[i],
> >> +					      videoNames[i])) {
> >> +			LOG(QcomCamss, Error) << "create_and_link failed";
> >> +			return false;
> >> +		}
> >> +
> >> +		pad = pipe_[i].subDevicePipe_[0]->entity()->getPadByIndex(0);
> >> +		if (pad)
> >> +			createCamera(&pipe_[i],
> >> +				     pad->links()[0]->source()->entity());
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >> +REGISTER_PIPELINE_HANDLER(PipelineHandlerQcomCamss);
> >> +
> >> +} /* namespace libcamera */
> >> \ No newline at end of file
> > 
> > Let's add one :-)
> > 
> ok

Hi Laurent,

>>  I have talked with Benjamin and we decided not to merge our works.
> 
> No worries, but I'm afraid that a qcom-camss-specific pipeline handler
> doesn't make much sense without ISP support. It came to my attention
> yesterday that work was ongoing on a pipeline handler for the i.MX SoCs,
> which could fall in the same category. We don't want to see a
> multiplication of nearly identical pipeline handlers for linear
> pipelines without an ISP, they should all be supported through the same
> code.
 So this means that it's useless to post a v2 since you won't
upstream it ?

Regards
Mickael

Hi Mickael,

On Wed, Jun 26, 2019 at 09:17:45AM +0000, Mickael GUENE wrote:
> Hi Laurent,
> 
> >> I have talked with Benjamin and we decided not to merge our works.
> > 
> > No worries, but I'm afraid that a qcom-camss-specific pipeline handler
> > doesn't make much sense without ISP support. It came to my attention
> > yesterday that work was ongoing on a pipeline handler for the i.MX SoCs,
> > which could fall in the same category. We don't want to see a
> > multiplication of nearly identical pipeline handlers for linear
> > pipelines without an ISP, they should all be supported through the same
> > code.
> 
> So this means that it's useless to post a v2 since you won't
> upstream it ?

Given our current understanding of the issue, yes, we won't merge a
qcom-camss pipeline handler as the qcom-camss should instead be
supported through a simple pipeline handler.

Laurent,

On 6/26/19 11:23, Laurent Pinchart wrote:
> Hi Mickael,
> 
> On Wed, Jun 26, 2019 at 09:17:45AM +0000, Mickael GUENE wrote:
>> Hi Laurent,
>>
>>>> I have talked with Benjamin and we decided not to merge our works.
>>>
>>> No worries, but I'm afraid that a qcom-camss-specific pipeline handler
>>> doesn't make much sense without ISP support. It came to my attention
>>> yesterday that work was ongoing on a pipeline handler for the i.MX SoCs,
>>> which could fall in the same category. We don't want to see a
>>> multiplication of nearly identical pipeline handlers for linear
>>> pipelines without an ISP, they should all be supported through the same
>>> code.
>>
>> So this means that it's useless to post a v2 since you won't
>> upstream it ?
> 
> Given our current understanding of the issue, yes, we won't merge a
> qcom-camss pipeline handler as the qcom-camss should instead be
> supported through a simple pipeline handler.
> 

 Thanks for the clarification. So I will continue to maintain my own
set of patches.

Regards
Mickael