[libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create CIO2Device class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 2 11:07:58 CEST 2019
Hi Jacopo,
Thank you for the patch.
On Tue, Mar 26, 2019 at 09:38:53AM +0100, Jacopo Mondi wrote:
> Group CIO2 components (cio2, csi2 and image sensor) in a class
> associated with the CameraData, to ease management and initialization of
> CIO2 unit at camera registration time. A CIO2 unit will always be associated
> with a single Camera only.
By the way, aren't commit messages supposed to be wrapped at 72 columns
?
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 320 +++++++++++++++------------
> 1 file changed, 175 insertions(+), 145 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 04cd02653711..21205b39afee 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -44,6 +44,32 @@ static int mediaBusToCIO2Format(unsigned int code)
> }
> }
>
> +class CIO2Device
> +{
> +public:
> + CIO2Device()
> + : output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> + {
> + }
> +
> + ~CIO2Device()
> + {
> + delete output_;
> + delete csi2_;
> + delete sensor_;
> + }
> +
> + const std::string &name() const;
> + int init(const MediaDevice *media, unsigned int index);
I would put init() first, to declare methods in the same order as
they're supposed to be called.
> +
> + V4L2Device *output_;
> + V4L2Subdevice *csi2_;
> + V4L2Subdevice *sensor_;
> +
> + /* Maximum sizes and the mbus code used to produce them. */
> + std::pair<unsigned int, SizeRange> maxSizes_;
> +};
> +
> class PipelineHandlerIPU3 : public PipelineHandler
> {
> public:
> @@ -67,36 +93,23 @@ public:
> bool match(DeviceEnumerator *enumerator);
>
> private:
> + static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +
> class IPU3CameraData : public CameraData
> {
> public:
> IPU3CameraData(PipelineHandler *pipe)
> - : CameraData(pipe), cio2_(nullptr), csi2_(nullptr),
> - sensor_(nullptr)
> - {
> - }
> -
> - ~IPU3CameraData()
> + : CameraData(pipe)
> {
> - delete cio2_;
> - delete csi2_;
> - delete sensor_;
> }
>
> void bufferReady(Buffer *buffer);
>
> - V4L2Device *cio2_;
> - V4L2Subdevice *csi2_;
> - V4L2Subdevice *sensor_;
> + CIO2Device cio2_;
>
> Stream stream_;
> -
> - /* Maximum sizes and the mbus code used to produce them. */
> - std::pair<unsigned int, SizeRange> maxSizes_;
> };
>
> - static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> -
> IPU3CameraData *cameraData(const Camera *camera)
> {
> return static_cast<IPU3CameraData *>(
> @@ -105,22 +118,22 @@ private:
>
> void registerCameras();
>
> - std::shared_ptr<MediaDevice> cio2_;
> - std::shared_ptr<MediaDevice> imgu_;
> + std::shared_ptr<MediaDevice> cio2MediaDev_;
> + std::shared_ptr<MediaDevice> imguMediaDev_;
I still dislike those longer new names, but I can live with our
disagreement on this topic, so please keep them :-)
> };
>
> PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> - : PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)
> + : PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
> {
> }
>
> PipelineHandlerIPU3::~PipelineHandlerIPU3()
> {
> - if (cio2_)
> - cio2_->release();
> + if (cio2MediaDev_)
> + cio2MediaDev_->release();
>
> - if (imgu_)
> - imgu_->release();
> + if (imguMediaDev_)
> + imguMediaDev_->release();
> }
>
> std::map<Stream *, StreamConfiguration>
> @@ -130,11 +143,12 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> std::map<Stream *, StreamConfiguration> configs;
> IPU3CameraData *data = cameraData(camera);
> StreamConfiguration *config = &configs[&data->stream_];
> - SizeRange &maxRange = data->maxSizes_.second;
> + CIO2Device *cio2 = &data->cio2_;
You could make this a const reference instead of a pointer (const isn't
strictly required, but as this function isn't supposed to modify
data->cio2_, it would help catching bugs).
> + SizeRange &maxRange = cio2->maxSizes_.second;
Same here, this could be const. If you prefer pointers that's OK too,
but I would then make both cio2 and maxRange pointers for consistency.
> config->width = maxRange.maxWidth;
> config->height = maxRange.maxHeight;
> - config->pixelFormat = data->maxSizes_.first;
> + config->pixelFormat = cio2->maxSizes_.first;
> config->bufferCount = IPU3_BUFFER_COUNT;
>
> LOG(IPU3, Debug)
> @@ -150,9 +164,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> {
> IPU3CameraData *data = cameraData(camera);
> StreamConfiguration *cfg = &config[&data->stream_];
> - V4L2Subdevice *sensor = data->sensor_;
> - V4L2Subdevice *csi2 = data->csi2_;
> - V4L2Device *cio2 = data->cio2_;
> + V4L2Subdevice *sensor = data->cio2_.sensor_;
> + V4L2Subdevice *csi2 = data->cio2_.csi2_;
> + V4L2Device *cio2 = data->cio2_.output_;
> V4L2SubdeviceFormat subdevFormat = {};
> V4L2DeviceFormat devFormat = {};
> int ret;
> @@ -209,13 +223,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>
> int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> {
> - IPU3CameraData *data = cameraData(camera);
> const StreamConfiguration &cfg = stream->configuration();
> + IPU3CameraData *data = cameraData(camera);
Is there a need for the reordering ?
> + V4L2Device *cio2 = data->cio2_.output_;
>
> if (!cfg.bufferCount)
> return -EINVAL;
>
> - int ret = data->cio2_->exportBuffers(&stream->bufferPool());
> + int ret = cio2->exportBuffers(&stream->bufferPool());
> if (ret) {
> LOG(IPU3, Error) << "Failed to request memory";
> return ret;
> @@ -227,8 +242,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2_.output_;
>
> - int ret = data->cio2_->releaseBuffers();
> + int ret = cio2->releaseBuffers();
> if (ret) {
> LOG(IPU3, Error) << "Failed to release memory";
> return ret;
> @@ -240,9 +256,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> int PipelineHandlerIPU3::start(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2_.output_;
> int ret;
>
> - ret = data->cio2_->streamOn();
> + ret = cio2->streamOn();
> if (ret) {
> LOG(IPU3, Info) << "Failed to start camera " << camera->name();
> return ret;
> @@ -254,8 +271,9 @@ int PipelineHandlerIPU3::start(Camera *camera)
> void PipelineHandlerIPU3::stop(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2_.output_;
>
> - if (data->cio2_->streamOff())
> + if (cio2->streamOff())
> LOG(IPU3, Info) << "Failed to stop camera " << camera->name();
>
> PipelineHandler::stop(camera);
> @@ -264,6 +282,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2_.output_;
> Stream *stream = &data->stream_;
>
> Buffer *buffer = request->findBuffer(stream);
> @@ -273,7 +292,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> return -ENOENT;
> }
>
> - int ret = data->cio2_->queueBuffer(buffer);
> + int ret = cio2->queueBuffer(buffer);
> if (ret < 0)
> return ret;
>
> @@ -312,17 +331,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> * It is safe to acquire both media devices at this point as
> * DeviceEnumerator::search() skips the busy ones for us.
> */
> - cio2_ = enumerator->search(cio2_dm);
> - if (!cio2_)
> + cio2MediaDev_ = enumerator->search(cio2_dm);
> + if (!cio2MediaDev_)
> return false;
>
> - cio2_->acquire();
> + cio2MediaDev_->acquire();
>
> - imgu_ = enumerator->search(imgu_dm);
> - if (!imgu_)
> + imguMediaDev_ = enumerator->search(imgu_dm);
> + if (!imguMediaDev_)
> return false;
>
> - imgu_->acquire();
> + imguMediaDev_->acquire();
>
> /*
> * Disable all links that are enabled by default on CIO2, as camera
> @@ -331,17 +350,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> * Close the CIO2 media device after, as links are enabled and should
> * not need to be changed after.
> */
> - if (cio2_->open())
> + if (cio2MediaDev_->open())
> return false;
>
> - if (cio2_->disableLinks()) {
> - cio2_->close();
> + if (cio2MediaDev_->disableLinks()) {
> + cio2MediaDev_->close();
> return false;
> }
>
> registerCameras();
>
> - cio2_->close();
> + cio2MediaDev_->close();
>
> return true;
> }
> @@ -355,115 +374,28 @@ void PipelineHandlerIPU3::registerCameras()
> {
> /*
> * For each CSI-2 receiver on the IPU3, create a Camera if an
> - * image sensor is connected to it.
> + * image sensor is connected to it and it can produce images in
s/and it/and the sensor/
> + * a compatible format.
> */
> unsigned int numCameras = 0;
> for (unsigned int id = 0; id < 4; ++id) {
> - std::string csi2Name = "ipu3-csi2 " + std::to_string(id);
> - MediaEntity *csi2 = cio2_->getEntityByName(csi2Name);
> - int ret;
> -
> - /*
> - * This shall not happen, as the device enumerator matched
> - * all entities described in the cio2_dm DeviceMatch.
> - *
> - * As this check is basically free, better stay safe than sorry.
> - */
> - if (!csi2)
> - continue;
> -
> - const std::vector<MediaPad *> &pads = csi2->pads();
> - if (pads.empty())
> - continue;
> -
> - /* IPU3 CSI-2 receivers have a single sink pad at index 0. */
> - MediaPad *sink = pads[0];
> - const std::vector<MediaLink *> &links = sink->links();
> - if (links.empty())
> - continue;
> -
> - /*
> - * Verify that the receiver is connected to a sensor, enable
> - * the media link between the two, and create a Camera with
> - * a unique name.
> - */
> - MediaLink *link = links[0];
> - MediaEntity *sensor = link->source()->entity();
> - if (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)
> - continue;
> -
> - if (link->setEnabled(true))
> - continue;
> -
> - std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>(this);
> -
> - std::string cameraName = sensor->name() + " " + std::to_string(id);
> + std::unique_ptr<IPU3CameraData> data =
> + utils::make_unique<IPU3CameraData>(this);
> std::set<Stream *> streams{ &data->stream_ };
> - std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
> -
> - /*
> - * Create and open video devices and subdevices associated with
> - * the camera.
> - *
> - * If any of these operations fails, the Camera instance won't
> - * be registered. The 'camera' shared pointer and the 'data'
> - * unique pointers go out of scope and delete the objects they
> - * manage.
> - */
> - std::string cio2Name = "ipu3-cio2 " + std::to_string(id);
> - MediaEntity *cio2 = cio2_->getEntityByName(cio2Name);
> - if (!cio2) {
> - LOG(IPU3, Error)
> - << "Failed to get entity '" << cio2Name << "'";
> - continue;
> - }
> -
> - data->cio2_ = new V4L2Device(cio2);
> - ret = data->cio2_->open();
> - if (ret)
> - continue;
> + CIO2Device *cio2 = &data->cio2_;
>
> - /*
> - * Make sure the sensor produces at least one image format
> - * compatible with IPU3 CIO2 requirements and cache the camera
> - * maximum sizes.
> - */
> - data->sensor_ = new V4L2Subdevice(sensor);
> - ret = data->sensor_->open();
> + int ret = cio2->init(cio2MediaDev_.get(), id);
> if (ret)
> continue;
>
> - for (auto it : data->sensor_->formats(0)) {
> - int mbusCode = mediaBusToCIO2Format(it.first);
> - if (mbusCode < 0)
> - continue;
> -
> - for (const SizeRange &size : it.second) {
> - SizeRange &maxSize = data->maxSizes_.second;
> -
> - if (maxSize.maxWidth < size.maxWidth &&
> - maxSize.maxHeight < size.maxHeight) {
> - maxSize.maxWidth = size.maxWidth;
> - maxSize.maxHeight = size.maxHeight;
> - data->maxSizes_.first = mbusCode;
> - }
> - }
> - }
> - if (data->maxSizes_.second.maxWidth == 0) {
> - LOG(IPU3, Info)
> - << "Sensor '" << data->sensor_->entityName()
> - << "' detected, but no supported image format "
> - << " found: skip camera creation";
> - continue;
> - }
> -
> - data->csi2_ = new V4L2Subdevice(csi2);
> - ret = data->csi2_->open();
> - if (ret)
> - continue;
> + std::string cameraName = cio2->name() + " "
> + + std::to_string(id);
> + std::shared_ptr<Camera> camera = Camera::create(this,
> + cameraName,
> + streams);
>
> - data->cio2_->bufferReady.connect(data.get(),
> - &IPU3CameraData::bufferReady);
> + cio2->output_->bufferReady.connect(data.get(),
> + &IPU3CameraData::bufferReady);
>
> registerCamera(std::move(camera), std::move(data));
>
> @@ -484,6 +416,104 @@ void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)
> pipe_->completeRequest(camera_, request);
> }
>
> +/*------------------------------------------------------------------------------
> + * CIO2 Device
> + */
> +
> +const std::string &CIO2Device::name() const
> +{
> + return sensor_->entityName();
This is dangerous, as you will crash if name() is called before init().
As this method is only called once, in
PipelineHandlerIPU3::registerCameras, I think you can inline
cio2->sensor_->entityName() there.
> +}
> +
> +/**
> + * \brief Initialize components of the CIO2 device with \a index
> + * \param[in] media The CIO2 media device
> + * \param[in] index The CIO2 device index
> + *
> + * Create and open the device and subdevices in the CIO2 instance at \a index,
s/the device/the video device/
> + * if an image sensor is connected to the CSI-2 receiver of this CIO2 instance.
> + * Enable the media links connecting the CIO2 components to prepare for capture
> + * operations and cache the sensor maximum sizes.
s/sizes/size/
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV No image sensor is connected to this CIO2 instance
s/No image sensor/No supported image sensor/
> + */
> +int CIO2Device::init(const MediaDevice *media, unsigned int index)
> +{
> + int ret;
> +
> + /* Verify that a sensor subdevice is connected to this CIO2 instance. */
"... and enable the link between the two." ?
> + std::string csi2Name = "ipu3-csi2 " + std::to_string(index);
> + MediaEntity *csi2Entity = media->getEntityByName(csi2Name);
> + const std::vector<MediaPad *> &pads = csi2Entity->pads();
> + if (pads.empty())
> + return -ENODEV;
> +
> + /* IPU3 CSI-2 receivers have a single sink pad at index 0. */
> + MediaPad *sink = pads[0];
> + const std::vector<MediaLink *> &links = sink->links();
> + if (links.empty())
> + return -ENODEV;
> +
> + MediaLink *link = links[0];
> + MediaEntity *sensorEntity = link->source()->entity();
> + if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)
> + return -ENODEV;
> +
> + ret = link->setEnabled(true);
> + if (ret)
> + return ret;
> +
> + /*
> + * Now that we're sure a sensor subdevice is connected, make sure it
> + * produces at least one image format compatible with CIO2 requirements
> + * and cache the camera maximum sizes.
s/sizes/size/
> + *
> + * \todo Define when to open and close video device nodes, as they
> + * might impact on power consumption.
> + */
> + sensor_ = new V4L2Subdevice(sensorEntity);
> + ret = sensor_->open();
> + if (ret)
> + return ret;
> +
> + for (auto it : sensor_->formats(0)) {
> + int mbusCode = mediaBusToCIO2Format(it.first);
> + if (mbusCode < 0)
> + continue;
> +
> + for (const SizeRange &size : it.second) {
> + SizeRange &maxRange = maxSizes_.second;
> +
> + if (maxRange.maxWidth < size.maxWidth &&
> + maxRange.maxHeight < size.maxHeight) {
> + maxRange.maxWidth = size.maxWidth;
> + maxRange.maxHeight = size.maxHeight;
> + maxSizes_.first = mbusCode;
> + }
> + }
> + }
> + if (maxSizes_.second.maxWidth == 0) {
> + LOG(IPU3, Info) << "Sensor '" << sensor_->entityName()
> + << "' detected, but no supported image format "
> + << " found: skip camera creation";
> + return -ENODEV;
> + }
As this code is moved from PipelineHandlerIPU3::registerCameras(), don't
forget to update it after taking the comments on the previous patches
into account.
> +
> + csi2_ = new V4L2Subdevice(csi2Entity);
> + ret = csi2_->open();
> + if (ret)
> + return ret;
> +
> + std::string cio2Name = "ipu3-cio2 " + std::to_string(index);
> + output_ = V4L2Device::fromEntityName(media, cio2Name);
> + ret = output_->open();
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>
> } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list