[libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create CIO2Device class
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 2 14:28:22 CEST 2019
Hi Niklas,
On Tue, Apr 02, 2019 at 01:22:22PM +0200, Niklas Söderlund wrote:
> 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?
>
All the fuss around returning a default config, then moving it to the
CIO2 device, then changing it again to use the format provided by the
ImgU, then default to a safe value for Soraka could be avoided if I
change streamConfiguration in a final last patch. Although that breaks
capture operations in the intermediate patches, which is in my opinion
acceptable as the series will be pushed all in one go, but has been
pointed out in a previous review round as an error.
If that's fine with everyone, I'll change streamConfiguration() once
at the end of the series.
> > 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.
>
I changed this as I have here introduced data->cio2_, while this here
above would still be PipelineHandlerIPU3::cio2_ and in a few places I
would have things like:
data->cio2_.init(cio2_, ..)
which is pretty confusing.
Laurent does not like the rename either, but I would like to not
confuse the CIO2Device with the CIO2 MediaDevice.
Thanks
j
> > };
> >
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190402/25f0a8d9/attachment-0001.sig>
More information about the libcamera-devel
mailing list