[libcamera-devel] [PATCH] libcamera: pipeline: Add Qualcomm 8x16 pipeline
Mickael GUENE
mickael.guene at st.com
Wed Jun 26 10:34:29 CEST 2019
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 at 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
More information about the libcamera-devel
mailing list