| Message ID | 20190320163055.22056-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Mar 20, 2019 at 05:30:29PM +0100, Jacopo Mondi wrote: > Group CIO2 devices (cio2, csi2 and image sensor) in a structure > associated with the CameraData, to ease management and initialize it at > camera registration time, as a CIO2 unit will always be associated with > a Camera only. > > While at there update the IPU3 pipeline handler implementation to avoid s/there/it/ > name clashes. Is there a name clash ? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 301 +++++++++++++++------------ > 1 file changed, 172 insertions(+), 129 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 1df074553fd1..2e351d04a784 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -27,6 +27,24 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +struct CIO2Device { > + CIO2Device() > + : output(nullptr), csi2(nullptr), sensor(nullptr) > + { > + } > + > + ~CIO2Device() > + { > + delete output; > + delete csi2; > + delete sensor; > + } > + > + V4L2Device *output; > + V4L2Subdevice *csi2; > + V4L2Subdevice *sensor; > +}; > + > class PipelineHandlerIPU3 : public PipelineHandler > { > public: > @@ -50,33 +68,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; Shouldn't this be cio2_ ? > > Stream stream_; > }; > > - static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > - > IPU3CameraData *cameraData(const Camera *camera) > { > return static_cast<IPU3CameraData *>( > @@ -85,24 +93,25 @@ private: > > int mediaBusToCIO2Format(unsigned int code); > > + int initCIO2(unsigned int index, CIO2Device *cio2); > void registerCameras(); > > - std::shared_ptr<MediaDevice> cio2_; > - std::shared_ptr<MediaDevice> imgu_; > + std::shared_ptr<MediaDevice> cio2MediaDev_; > + std::shared_ptr<MediaDevice> imguMediaDev_; I'm not a big fan of these new names and I would have skipped the rename, but I understand the rationale, and it's your code :-) > }; > > 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> > @@ -111,7 +120,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera, > { > std::map<Stream *, StreamConfiguration> configs; > IPU3CameraData *data = cameraData(camera); > - V4L2Subdevice *sensor = data->sensor_; > + V4L2Subdevice *sensor = data->cio2.sensor; > StreamConfiguration *config = &configs[&data->stream_]; > unsigned int bestSize = 0; > > @@ -152,9 +161,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; > @@ -211,13 +220,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; > @@ -229,8 +239,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; > @@ -242,9 +253,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; > @@ -256,8 +268,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); > @@ -266,6 +279,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); > @@ -275,7 +289,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; > > @@ -314,17 +328,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 > @@ -333,17 +347,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; > } > @@ -364,6 +378,111 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) > } > } > > +/** > + * \brief Initialize components of the CIO2 device \a index used by a camera > + * \param index The CIO2 device index > + * \param cio2 Pointer to the CIO2 instance > + * > + * 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. > + * > + * \return 0 on success or a negative error code otherwise > + * \retval -EINVAL Failure to create or open a video device or subdevice > + * \retval -ENODEV No image sensor is connected to this CIO2 instance > + */ > +int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2) > +{ > + std::string cio2Name = "ipu3-cio2 " + std::to_string(index); > + std::string csi2Name = "ipu3-csi2 " + std::to_string(index); You can declare those two variables at their respective use sites. > + int ret; > + > + /* Verify a sensor subdevice is connected to this CIO2 instance. */ s/Verify/Verify that/ (I'd also drop "subdevice", strictly speaking you're checking entities only.) > + MediaEntity *entity = cio2MediaDev_->getEntityByName(csi2Name); I'd name the variable csi2 (and the one below cio2), that will make the code easier to read. > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << csi2Name << "'"; This really can't happen, so I think you can drop the message. > + return -EINVAL; > + } > + > + const std::vector<MediaPad *> &pads = entity->pads(); > + if (pads.empty()) > + return -EINVAL; > + > + /* 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 -EINVAL; > + > + MediaLink *link = links[0]; > + MediaEntity *sensorEntity = link->source()->entity(); > + if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR) > + return -ENODEV; Shouldn't all the four errors above 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. > + * > + * \todo Define when to open and close video device nodes, as they > + * might impact on power consumption. s/impact on/impact/ > + */ > + cio2->sensor = new V4L2Subdevice(sensorEntity); > + ret = cio2->sensor->open(); > + if (ret) { > + delete cio2->sensor; > + > + return ret; > + } > + > + const FormatEnum formats = cio2->sensor->formats(0); > + auto it = formats.begin(); > + for (; it != formats.end(); ++it) { for (auto it : formats) > + if (mediaBusToCIO2Format(it->first) != -EINVAL) > + break; > + } > + if (it == formats.end()) { > + LOG(IPU3, Info) << "Sensor '" << cio2->sensor->deviceName() > + << "' detected, but no supported image format " > + << " found: skip camera creation"; > + goto error_delete_sensor; > + } > + > + cio2->csi2 = new V4L2Subdevice(entity); > + ret = cio2->csi2->open(); > + if (ret) > + goto error_delete_csi2; > + > + entity = cio2MediaDev_->getEntityByName(cio2Name); > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << cio2Name << "'"; You could remove this message too as it really can't happen (the match operation ensured that we have corresponding entities). > + ret = -EINVAL; > + goto error_delete_csi2; > + } I'd do this before calling cio2->csi2->open(). > + > + cio2->output = new V4L2Device(entity); > + ret = cio2->output->open(); > + if (ret) > + goto error_delete_output; > + > + return 0; > + > +error_delete_output: > + delete cio2->output; > +error_delete_csi2: > + delete cio2->csi2; > +error_delete_sensor: > + delete cio2->sensor; Is this needed, doesn't the cio2 destructor handle this for you ? > + > + return ret; > +} > + > /* > * Cameras are created associating an image sensor (represented by a > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > @@ -377,100 +496,24 @@ void PipelineHandlerIPU3::registerCameras() > */ > 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; > + int ret; > > - /* > - * Make sure the sensor produces at least one image format > - * compatible with IPU3 CIO2 requirements. > - */ > - data->sensor_ = new V4L2Subdevice(sensor); > - ret = data->sensor_->open(); > + ret = initCIO2(id, cio2); > if (ret) > continue; > > - const FormatEnum formats = data->sensor_->formats(0); > - auto it = formats.begin(); > - for (; it != formats.end(); ++it) { > - if (mediaBusToCIO2Format(it->first) != -EINVAL) > - break; > - } > - if (it == formats.end()) { > - LOG(IPU3, Info) > - << "Sensor '" << data->sensor_->deviceName() > - << "' 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->sensor->deviceName() + " " > + + 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)); I wonder if it would make sense to move the code above to initCIO2() and rename the function registerCamera(). >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 1df074553fd1..2e351d04a784 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -27,6 +27,24 @@ namespace libcamera { LOG_DEFINE_CATEGORY(IPU3) +struct CIO2Device { + CIO2Device() + : output(nullptr), csi2(nullptr), sensor(nullptr) + { + } + + ~CIO2Device() + { + delete output; + delete csi2; + delete sensor; + } + + V4L2Device *output; + V4L2Subdevice *csi2; + V4L2Subdevice *sensor; +}; + class PipelineHandlerIPU3 : public PipelineHandler { public: @@ -50,33 +68,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_; }; - static constexpr unsigned int IPU3_BUFFER_COUNT = 4; - IPU3CameraData *cameraData(const Camera *camera) { return static_cast<IPU3CameraData *>( @@ -85,24 +93,25 @@ private: int mediaBusToCIO2Format(unsigned int code); + int initCIO2(unsigned int index, CIO2Device *cio2); void registerCameras(); - std::shared_ptr<MediaDevice> cio2_; - std::shared_ptr<MediaDevice> imgu_; + std::shared_ptr<MediaDevice> cio2MediaDev_; + std::shared_ptr<MediaDevice> imguMediaDev_; }; 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> @@ -111,7 +120,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera, { std::map<Stream *, StreamConfiguration> configs; IPU3CameraData *data = cameraData(camera); - V4L2Subdevice *sensor = data->sensor_; + V4L2Subdevice *sensor = data->cio2.sensor; StreamConfiguration *config = &configs[&data->stream_]; unsigned int bestSize = 0; @@ -152,9 +161,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; @@ -211,13 +220,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; @@ -229,8 +239,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; @@ -242,9 +253,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; @@ -256,8 +268,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); @@ -266,6 +279,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); @@ -275,7 +289,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; @@ -314,17 +328,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 @@ -333,17 +347,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; } @@ -364,6 +378,111 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) } } +/** + * \brief Initialize components of the CIO2 device \a index used by a camera + * \param index The CIO2 device index + * \param cio2 Pointer to the CIO2 instance + * + * 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. + * + * \return 0 on success or a negative error code otherwise + * \retval -EINVAL Failure to create or open a video device or subdevice + * \retval -ENODEV No image sensor is connected to this CIO2 instance + */ +int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2) +{ + std::string cio2Name = "ipu3-cio2 " + std::to_string(index); + std::string csi2Name = "ipu3-csi2 " + std::to_string(index); + int ret; + + /* Verify a sensor subdevice is connected to this CIO2 instance. */ + MediaEntity *entity = cio2MediaDev_->getEntityByName(csi2Name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << csi2Name << "'"; + return -EINVAL; + } + + const std::vector<MediaPad *> &pads = entity->pads(); + if (pads.empty()) + return -EINVAL; + + /* 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 -EINVAL; + + 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. + * + * \todo Define when to open and close video device nodes, as they + * might impact on power consumption. + */ + cio2->sensor = new V4L2Subdevice(sensorEntity); + ret = cio2->sensor->open(); + if (ret) { + delete cio2->sensor; + + return ret; + } + + const FormatEnum formats = cio2->sensor->formats(0); + auto it = formats.begin(); + for (; it != formats.end(); ++it) { + if (mediaBusToCIO2Format(it->first) != -EINVAL) + break; + } + if (it == formats.end()) { + LOG(IPU3, Info) << "Sensor '" << cio2->sensor->deviceName() + << "' detected, but no supported image format " + << " found: skip camera creation"; + goto error_delete_sensor; + } + + cio2->csi2 = new V4L2Subdevice(entity); + ret = cio2->csi2->open(); + if (ret) + goto error_delete_csi2; + + entity = cio2MediaDev_->getEntityByName(cio2Name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << cio2Name << "'"; + ret = -EINVAL; + goto error_delete_csi2; + } + + cio2->output = new V4L2Device(entity); + ret = cio2->output->open(); + if (ret) + goto error_delete_output; + + return 0; + +error_delete_output: + delete cio2->output; +error_delete_csi2: + delete cio2->csi2; +error_delete_sensor: + delete cio2->sensor; + + return ret; +} + /* * Cameras are created associating an image sensor (represented by a * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four @@ -377,100 +496,24 @@ void PipelineHandlerIPU3::registerCameras() */ 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; + int ret; - /* - * Make sure the sensor produces at least one image format - * compatible with IPU3 CIO2 requirements. - */ - data->sensor_ = new V4L2Subdevice(sensor); - ret = data->sensor_->open(); + ret = initCIO2(id, cio2); if (ret) continue; - const FormatEnum formats = data->sensor_->formats(0); - auto it = formats.begin(); - for (; it != formats.end(); ++it) { - if (mediaBusToCIO2Format(it->first) != -EINVAL) - break; - } - if (it == formats.end()) { - LOG(IPU3, Info) - << "Sensor '" << data->sensor_->deviceName() - << "' 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->sensor->deviceName() + " " + + 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));
Group CIO2 devices (cio2, csi2 and image sensor) in a structure associated with the CameraData, to ease management and initialize it at camera registration time, as a CIO2 unit will always be associated with a Camera only. While at there update the IPU3 pipeline handler implementation to avoid name clashes. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 301 +++++++++++++++------------ 1 file changed, 172 insertions(+), 129 deletions(-)