[libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create CIO2Device class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Apr 2 13:22:22 CEST 2019
Hi Jacopo,
Thanks for your work.
On 2019-03-26 09:38:53 +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.
I like the thing this patch does as it makes things more clean in the
end.
>
> 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);
> +
> + 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;
> -
This was introduced in the previous patch count you not move this change
to that patch and not move again here? Would it make sens to swap this
and the previous patch in this series? First move and break things out
and then add new features on top?
> 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 would move this rename to its own commit to make this patch easier to
review. You could tuck some of the reordering of declarations you do
into such a clean up patch also if you fancy.
> };
>
> 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_;
> + SizeRange &maxRange = cio2->maxSizes_.second;
>
> 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);
> + 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
> + * 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();
> +}
> +
> +/**
> + * \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,
> + * 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.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV No image sensor is connected to this CIO2 instance
> + */
> +int CIO2Device::init(const MediaDevice *media, unsigned int index)
> +{
> + int ret;
> +
> + /* Verify that a sensor subdevice is connected to this CIO2 instance. */
> + 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.
> + *
> + * \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;
> + }
> +
> + 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 */
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list