[libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline: Add RaspberryPi handler
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Aug 9 13:19:17 CEST 2019
Hi Laurent,
On 08/08/2019 22:51, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Thu, Aug 08, 2019 at 04:12:21PM +0100, Kieran Bingham wrote:
>> Utilise the CameraSensor class and construct a pipeline for a single
>> sensor on the Unicam, routed through the V4L2 Codec ISP interface.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> A pipeline handler for the RPi, very nice :-)
A "Proof of Concept" pipeline handler :-) - It works - but needs more
development and requires out of tree drivers for the kernel, and
out-of-driver patches for the out of tree drivers :S
We'll get there :-D
Thanks for the review comments. I've pretty much taken most of them in.
>> ---
>> src/libcamera/pipeline/meson.build | 1 +
>> src/libcamera/pipeline/raspberrypi.cpp | 425 +++++++++++++++++++++++++
>> 2 files changed, 426 insertions(+)
>> create mode 100644 src/libcamera/pipeline/raspberrypi.cpp
>>
>> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
>> index 0d466225a72e..7ed7b26f3033 100644
>> --- a/src/libcamera/pipeline/meson.build
>> +++ b/src/libcamera/pipeline/meson.build
>> @@ -1,4 +1,5 @@
>> libcamera_sources += files([
>> + 'raspberrypi.cpp',
>
> I would expect the same level of complexity as the IPU3 and RkISP1
> pipeline handlers, should this get its own directory ?
As yet, I've seen only a single file for the IPU3, and RKISP1.
What other files are expected? The IPA code will go in a separate
directory right? So I'm not yet sure what will be broken out in the
pipeline handlers to need their own directory.
(Except the IPU3 - That looks like it could be split to have an
ipu3.cpp, imgu.cpp, cio2.cpp.)
>
>> 'uvcvideo.cpp',
>> 'vimc.cpp',
>> ])
>> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp
>> new file mode 100644
>> index 000000000000..4c4c3dea0113
>> --- /dev/null
>> +++ b/src/libcamera/pipeline/raspberrypi.cpp
>> @@ -0,0 +1,425 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * raspberrypi.cpp - Pipeline handler for raspberrypi devices
>
> s/raspberrypi devices/Raspberry Pi devices/ ?
>
> Should we name it Raspberry Pi, or based on the SoC ? It could be used
> on other systems using the same SoC (or family of SoCs).
I expect this pipeline handler to function on all of the Raspberry Pi's
with a camera interface. This covers multiple SoCs.
So lets stick with RaspberryPi as the name I think.
>> + */
>> +
>> +#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_controls.h"
>> +#include "v4l2_videodevice.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(RPI)
>> +
>> +class RPiCameraData : public CameraData
>> +{
>> +public:
>> + RPiCameraData(PipelineHandler *pipe)
>> + : CameraData(pipe), sensor_(nullptr), unicam_(nullptr),
>> + isp_(nullptr)
>> + {
>> + }
>> +
>> + ~RPiCameraData()
>> + {
>> + bayerBuffers_.destroyBuffers();
>
> Shouldn't you also delete sensor_ ?
Yup.
>
>> + delete unicam_;
>> + delete isp_;
>> + }
>> +
>> + void sensorReady(Buffer *buffer);
>> + void ispOutputReady(Buffer *buffer);
>> + void ispCaptureReady(Buffer *buffer);
>> +
>> + CameraSensor *sensor_;
>> + V4L2VideoDevice *unicam_;
>> + V4L2M2MDevice *isp_;
>> + Stream stream_;
>> +
>> + BufferPool bayerBuffers_;
>> + std::vector<std::unique_ptr<Buffer>> rawBuffers_;
>> +};
>> +
>> +class RPiCameraConfiguration : public CameraConfiguration
>> +{
>> +public:
>> + RPiCameraConfiguration();
>> +
>> + Status validate() override;
>> +};
>> +
>> +class PipelineHandlerRPi : public PipelineHandler
>> +{
>> +public:
>> + PipelineHandlerRPi(CameraManager *manager);
>> + ~PipelineHandlerRPi();
>> +
>> + 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:
>> + RPiCameraData *cameraData(const Camera *camera)
>> + {
>> + return static_cast<RPiCameraData *>(
>> + PipelineHandler::cameraData(camera));
>> + }
>> +
>> + std::shared_ptr<MediaDevice> unicam_;
>> + std::shared_ptr<MediaDevice> codec_;
>> +};
>> +
>> +RPiCameraConfiguration::RPiCameraConfiguration()
>> + : CameraConfiguration()
>> +{
>> +}
>> +
>> +CameraConfiguration::Status RPiCameraConfiguration::validate()
>> +{
>> + Status status = Valid;
>> +
>> + if (config_.empty())
>> + return Invalid;
>> +
>> + /* Todo: Experiment with increased stream support through the ISP. */
>
> s/Todo:/\todo/
>
>> + if (config_.size() > 1) {
>> + config_.resize(1);
>> + status = Adjusted;
>> + }
>> +
>> + StreamConfiguration &cfg = config_[0];
>> +
>> + /* Todo: restrict to hardware capabilities. */
>
> I think this one should be addressed before merging the code.
Yes, needs more development.
>
>> +
>> + cfg.bufferCount = 4;
>> +
>> + return status;
>> +}
>> +
>> +PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
>> + : PipelineHandler(manager), unicam_(nullptr), codec_(nullptr)
>> +{
>> +}
>> +
>> +PipelineHandlerRPi::~PipelineHandlerRPi()
>> +{
>> + if (unicam_)
>> + unicam_->release();
>> +
>> + if (codec_)
>> + codec_->release();
>> +}
>> +
>> +CameraConfiguration *
>> +PipelineHandlerRPi::generateConfiguration(Camera *camera,
>> + const StreamRoles &roles)
>> +{
>> + CameraConfiguration *config = new RPiCameraConfiguration();
>> +
>> + if (roles.empty())
>> + return config;
>> +
>> + StreamConfiguration cfg{};
>> + cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
>> + cfg.size = { 1920, 1080 }; // data->sensor_->resolution();
>
> Let's remove commented-out code.
>
> What prevents from using the sensor resolution, and how is 1080p
> selected as the default resolution ?
The RaspberryPi v2 camera defaults as a 3280x2464 output.
The ISP has currently got an (incorrect) artificial limitation of 1080p.
We can work through this when we get updated ISP support.
>> + cfg.bufferCount = 4;
>> +
>> + LOG(RPI, Debug) << "Config default as " << cfg.toString();
>
> I think the configuration is printed in the caller already.
>
Removed.
>> +
>> + config->addConfiguration(cfg);
>> +
>> + config->validate();
>> +
>> + return config;
>> +}
>> +
>> +int PipelineHandlerRPi::configure(Camera *camera,
>> + CameraConfiguration *config)
>
> This holds on a single line.
>
>> +{
>> + RPiCameraData *data = cameraData(camera);
>> + StreamConfiguration &cfg = config->at(0);
>> + int ret;
>> +
>> + Size sensorSize = { 1920, 1080 };
>> + Size outputSize = { 1920, 1088 };
>> +
>> + V4L2DeviceFormat format = {};
>> + format.size = sensorSize;
>> + format.fourcc = V4L2_PIX_FMT_SRGGB10P;
>
> This all seems to lack genericity :-) I think that at least he format
> should support different Bayer patterns and bpp values.
>
> Don't you need to set the format on the sensor subdev ?
>
>> +
>> + LOG(RPI, Debug) << "Setting format to " << format.toString();
>> +
>> + ret = data->unicam_->setFormat(&format);
>> + if (ret)
>> + return ret;
>> +
>> + if (format.size != sensorSize ||
>> + format.fourcc != V4L2_PIX_FMT_SRGGB10P) {
>> + LOG(RPI, Error)
>> + << "Failed to set format on Video device: "
>> + << format.toString();
>> + return -EINVAL;
>> + }
>> +
>> + format.size = outputSize;
>> +
>> + ret = data->isp_->output()->setFormat(&format);
>> +
>> + if (format.size != outputSize ||
>> + format.fourcc != V4L2_PIX_FMT_SRGGB10P) {
>> + LOG(RPI, Error) << "Failed to set format on ISP output device: "
>> + << format.toString();
>> + return -EINVAL;
>> + }
>> +
>> + /* Configure the ISP based to generate the requested size and format. */
>
> Are you missing something after "based" ?
s/based//
Or it could simply be "Configure the ISP" ... but we have limited
configuration so far...
>
>> + format.size = cfg.size;
>> + format.fourcc = cfg.pixelFormat;
>> +
>> + ret = data->isp_->capture()->setFormat(&format);
>> +
>> + if (format.size != cfg.size ||
>> + format.fourcc != cfg.pixelFormat) {
>> + LOG(RPI, Error)
>> + << "Failed to set format on ISP capture device: "
>> + << format.toString();
>> + return -EINVAL;
>> + }
>> +
>> + cfg.setStream(&data->stream_);
>> +
>> + return 0;
>> +}
>> +
>> +int PipelineHandlerRPi::allocateBuffers(Camera *camera,
>> + const std::set<Stream *> &streams)
>> +{
>> + RPiCameraData *data = cameraData(camera);
>> + Stream *stream = *streams.begin();
>> + const StreamConfiguration &cfg = stream->configuration();
>> + int ret;
>> +
>> + LOG(RPI, Debug) << "Requesting " << cfg.bufferCount << " buffers";
>
> I'd drop this message, or move it to Camera::allocateBuffers().
Its stream dependent ... so I'll just drop it.
>
>> +
>> + /*
>> + * Buffers are allocated on the camera, and the capture pad of the ISP
>> + * unicam -> isp.output -> isp.capture -> Application
>> + */
>> +
>> + /* Create a new intermediate buffer pool */
>
> s/pool/pool./ (same comment for all other comments in this file that
> don't end with a period)
Do you think you can make a rule for this in checkstyle.py?
>> + data->bayerBuffers_.createBuffers(cfg.bufferCount);
>> +
>> + /* Tie the unicam video buffers to the intermediate pool */
>> + ret = data->unicam_->exportBuffers(&data->bayerBuffers_);
>> + if (ret)
>> + return ret;
>> +
>> + ret = data->isp_->output()->importBuffers(&data->bayerBuffers_);
>> + if (ret)
>> + return ret;
>> +
>> + /* Tie the stream buffers to the capture device of the ISP */
>> + if (stream->memoryType() == InternalMemory)
>> + ret = data->isp_->capture()->exportBuffers(&stream->bufferPool());
>> + else
>> + ret = data->isp_->capture()->importBuffers(&stream->bufferPool());
>> +
>> + return ret;
>> +}
>> +
>> +int PipelineHandlerRPi::freeBuffers(Camera *camera,
>> + const std::set<Stream *> &streams)
>> +{
>> + RPiCameraData *data = cameraData(camera);
>> + int ret;
>> +
>> + ret = data->unicam_->releaseBuffers();
>> + ret = data->isp_->output()->releaseBuffers();
>> + ret = data->isp_->capture()->releaseBuffers();
>
> You're losing the first two error values.
Yup - quick and dirty PoC :D
There are three cleanup operations. If one fails - should the other
still be attempted? Or just give up ?
>
>> +
>> + data->bayerBuffers_.destroyBuffers();
>> +
>> + return ret;
>> +}
>> +
>> +int PipelineHandlerRPi::start(Camera *camera)
>> +{
>> + RPiCameraData *data = cameraData(camera);
>> + int ret;
>> +
>> + data->rawBuffers_ = data->unicam_->queueAllBuffers();
>> + if (data->rawBuffers_.empty()) {
>> + LOG(RPI, Debug) << "Failed to queue unicam buffers";
>> + return -EINVAL;
>> + }
>> +
>> + LOG(RPI, Warning) << "Using hard-coded exposure/gain defaults";
>> +
>> + V4L2ControlList controls;
>> + controls.add(V4L2_CID_EXPOSURE, 1700);
>> + controls.add(V4L2_CID_ANALOGUE_GAIN, 180);
>> + ret = data->unicam_->setControls(&controls);
>> + if (ret) {
>> + LOG(RPI, Error) << "Failed to set controls";
>> + return ret;
>> + }
>> +
>> + ret = data->isp_->output()->streamOn();
>> + if (ret)
>> + return ret;
>> +
>> + ret = data->isp_->capture()->streamOn();
>> + if (ret)
>> + return ret;
>> +
>> + ret = data->unicam_->streamOn();
>> + if (ret)
>> + return ret;
>
> Shouldn't you stop streaming on the successfully started streams when an
> error happens ?
>
>> +
>> + return ret;
>
> return 0;
>
>> +}
>> +
>> +void PipelineHandlerRPi::stop(Camera *camera)
>> +{
>> + RPiCameraData *data = cameraData(camera);
>> +
>> + data->isp_->capture()->streamOff();
>> + data->isp_->output()->streamOff();
>> + data->unicam_->streamOff();
>> +
>> + data->rawBuffers_.clear();
>> +}
>> +
>> +int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)
>> +{
>> + RPiCameraData *data = cameraData(camera);
>> + Buffer *buffer = request->findBuffer(&data->stream_);
>> + if (!buffer) {
>> + LOG(RPI, Error)
>> + << "Attempt to queue request with invalid stream";
>> +
>> + return -ENOENT;
>
> Can this happen ?
I don't know - I think this was taken from the new buffer handling bits
which have changed quite a lot lately.
This code exactly mirrors the queueRequest in the RkISP1, and Vimc.
>
>> + }
>> +
>> + int ret = data->isp_->capture()->queueBuffer(buffer);
>> + if (ret < 0)
>> + return ret;
>> +
>> + PipelineHandler::queueRequest(camera, request);
>> +
>> + return 0;
>> +}
>> +
>> +bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>> +{
>> + DeviceMatch unicam("unicam");
>> + DeviceMatch codec("bcm2835-codec");
>> +
>> + /* We explicitly need the ISP device from the MMAL codec driver. */
>> + codec.add("bcm2835-codec-isp");
>
> Is there any subdev in the unicam device that we could match on ?
Possibly one called "unicam" ... should we match "unicam":"unicam" ? or
is "unicam" sufficient...
I'll add:
/* The video node is also named unicam. */
unicam.add("unicam");
>> +
>> + unicam_ = enumerator->search(unicam);
>> + if (!unicam_)
>> + return false;
>> +
>> + codec_ = enumerator->search(codec);
>> + if (!codec_)
>> + return false;
>> +
>> + unicam_->acquire();
>> + codec_->acquire();
>> +
>> + std::unique_ptr<RPiCameraData> data = utils::make_unique<RPiCameraData>(this);
>> +
>> + /* Locate and open the unicam video node. */
>> + data->unicam_ = new V4L2VideoDevice(unicam_->getEntityByName("unicam"));
>> + if (data->unicam_->open())
>> + return false;
>> +
>> + /* Locate the ISP M2M node */
>> + MediaEntity *isp = codec_->getEntityByName("bcm2835-codec-isp");
>> + if (!isp)
>> + return false;
>> +
>> + data->isp_ = new V4L2M2MDevice(isp->deviceNode());
>> + if (data->isp_->status()) {
>> + LOG(RPI, Error) << "Could not create the ISP device";
>> + return false;
>> + }
>> +
>> + data->unicam_->bufferReady.connect(data.get(), &RPiCameraData::sensorReady);
>> + data->isp_->output()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputReady);
>> + data->isp_->capture()->bufferReady.connect(data.get(), &RPiCameraData::ispCaptureReady);
>> +
>> + /* Identify the sensor */
>> + for (MediaEntity *entity : unicam_->entities()) {
>> + if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
>> + data->sensor_ = new CameraSensor(entity);
>> + break;
>> + }
>> + }
>> +
>> + if (!data->sensor_)
>> + return false;
>> +
>> + int ret = data->sensor_->init();
>> + if (ret)
>> + return false;
>
> No need for an intermediate variable, you can write
>
> if (data->sensor_->init())
> return false;
done
>> +
>> + /* Create and register the camera. */
>> + std::set<Stream *> streams{ &data->stream_ };
>> + std::shared_ptr<Camera> camera =
>> + Camera::create(this, data->sensor_->entity()->name(), streams);
>> + registerCamera(std::move(camera), std::move(data));
>> +
>> + return true;
>> +}
>> +
>> +void RPiCameraData::sensorReady(Buffer *buffer)
>> +{
>> + /* Deliver the frame from the sensor to the ISP */
>
> You should skip this when buffer->status() == Buffer::BufferCancelled
> (see the IPU3 pipeline handler).
Hrm ... I had this ... I must have lost them along a rebase.
>
>> + isp_->output()->queueBuffer(buffer);
>> +}
>> +
>> +void RPiCameraData::ispOutputReady(Buffer *buffer)
>> +{
>> + /* Return a completed buffer from the ISP back to the sensor */
>
> Same comment here.
>
>> + unicam_->queueBuffer(buffer);
>> +}
>> +
>> +void RPiCameraData::ispCaptureReady(Buffer *buffer)
>> +{
>> + Request *request = buffer->request();
>> +
>> + pipe_->completeBuffer(camera_, request, buffer);
>> + pipe_->completeRequest(camera_, request);
>> +}
>> +
>> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi);
>> +
>> +} /* namespace libcamera */
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list