| Message ID | 20190228200410.3022-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Feb 28, 2019 at 09:04:01PM +0100, Jacopo Mondi wrote: > Group CIO2 devices (cio2, csi2 and image sensor) in a structure > associated with the CameraData, to ease management of the CIO2 devices. > > Update the IPU3 pipeline handler implementation to avoid name clashes > and break-out IPU3CameraData from the pipeline handler class for > clarity. I'm not sure that really brings clarity :-) I would have kept the IPU3CameraData structure where it was, but preferences differ of course. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 251 ++++++++++++++------------- > 1 file changed, 133 insertions(+), 118 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9694d0ce51ab..d3f1d9a95f81 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -24,6 +24,30 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +struct Cio2Device { Should this be called CIO2Device ? > + Cio2Device() > + : output(nullptr), csi2(nullptr), sensor(nullptr) {} I'd open the {} on the next two lines for consistency, up to you. > + > + ~Cio2Device() > + { > + delete output; > + delete csi2; > + delete sensor; > + } I'm not very fond on having part of the logic in the PipelineHandlerIPU3 class (the initCio2() function - I know I asked for it to be moved there) part here in the destructor :-/ Splitting it in two doesn't feel really right, I'd move these three delete statements to the IPU3CameraData destructor. What do you think ? > + > + V4L2Device *output; > + V4L2Subdevice *csi2; > + V4L2Subdevice *sensor; > +}; > + > +class IPU3CameraData : public CameraData > +{ > +public: > + Cio2Device cio2; > + > + Stream stream_; > +}; > + > class PipelineHandlerIPU3 : public PipelineHandler > { > public: > @@ -47,65 +71,47 @@ public: > bool match(DeviceEnumerator *enumerator); > > private: > - class IPU3CameraData : public CameraData > - { > - public: > - IPU3CameraData() > - : cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {} > - > - ~IPU3CameraData() > - { > - delete cio2_; > - delete csi2_; > - delete sensor_; > - } > - > - V4L2Device *cio2_; > - V4L2Subdevice *csi2_; > - V4L2Subdevice *sensor_; > - > - Stream stream_; > - }; > - > IPU3CameraData *cameraData(const Camera *camera) > { > return static_cast<IPU3CameraData *>( > PipelineHandler::cameraData(camera)); > } > > + int initCio2(unsigned int index, Cio2Device *cio2); initCIO2() ? > void registerCameras(); > > - std::shared_ptr<MediaDevice> cio2_; > - std::shared_ptr<MediaDevice> imgu_; > + std::shared_ptr<MediaDevice> cio2MediaDev_; > + std::shared_ptr<MediaDevice> imguMediaDev_; Are these renames really needed ? > }; > > 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> > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > std::vector<Stream *> &streams) > { > - IPU3CameraData *data = cameraData(camera); > std::map<Stream *, StreamConfiguration> configs; > + IPU3CameraData *data = cameraData(camera); > + V4L2Subdevice *sensor = data->cio2.sensor; > V4L2SubdeviceFormat format = {}; > > /* > * FIXME: As of now, return the image format reported by the sensor. > * In future good defaults should be provided for each stream. > */ > - if (data->sensor_->getFormat(0, &format)) { > + if (sensor->getFormat(0, &format)) { > LOG(IPU3, Error) << "Failed to create stream configurations"; > return configs; > } > @@ -126,9 +132,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; > @@ -185,13 +191,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; > @@ -203,8 +210,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; > @@ -216,9 +224,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > int PipelineHandlerIPU3::start(const 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; > @@ -230,14 +239,16 @@ int PipelineHandlerIPU3::start(const Camera *camera) > void PipelineHandlerIPU3::stop(const 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(); > } > > int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request) > { > IPU3CameraData *data = cameraData(camera); > + V4L2Device *cio2 = data->cio2.output; > Stream *stream = &data->stream_; > > Buffer *buffer = request->findBuffer(stream); > @@ -247,7 +258,7 @@ int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request) > return -ENOENT; > } > > - data->cio2_->queueBuffer(buffer); > + cio2->queueBuffer(buffer); > > return 0; > } > @@ -278,20 +289,20 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > imgu_dm.add("ipu3-imgu 1 viewfinder"); > imgu_dm.add("ipu3-imgu 1 3a stat"); > > - cio2_ = enumerator->search(cio2_dm); > - if (!cio2_) > + cio2MediaDev_ = enumerator->search(cio2_dm); > + if (!cio2MediaDev_) > return false; > > - imgu_ = enumerator->search(imgu_dm); > - if (!imgu_) > + imguMediaDev_ = enumerator->search(imgu_dm); > + if (!imguMediaDev_) > return false; > > /* > * It is safe to acquire both media devices at this point as > * DeviceEnumerator::search() skips the busy ones for us. > */ > - cio2_->acquire(); > - imgu_->acquire(); > + cio2MediaDev_->acquire(); > + imguMediaDev_->acquire(); > > /* > * Disable all links that are enabled by default on CIO2, as camera > @@ -300,28 +311,90 @@ 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()) > goto error_release_mdev; > > - if (cio2_->disableLinks()) > + if (cio2MediaDev_->disableLinks()) > goto error_close_cio2; > > registerCameras(); > > - cio2_->close(); > + cio2MediaDev_->close(); > > return true; > > error_close_cio2: > - cio2_->close(); > + cio2MediaDev_->close(); > > error_release_mdev: > - cio2_->release(); > - imgu_->release(); > + cio2MediaDev_->release(); > + imguMediaDev_->release(); > > return false; > } > This pipeline handler will likely be used as a reference implementation in the future, could you add more documentation, even for private functions ? > +int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > +{ > + int ret; > + > + /* Verify a sensor subdevice is connected to this CIO2 instance. */ > + std::string csi2Name = "ipu3-csi2 " + std::to_string(index); > + MediaEntity *entity = cio2MediaDev_->getEntityByName(csi2Name); > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << csi2Name << "'"; > + return -ENODEV; > + } > + > + 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, create and open > + * video devices and subdevices associated with this CIO2 unit. > + */ > + cio2->sensor = new V4L2Subdevice(sensorEntity); > + ret = cio2->sensor->open(); > + if (ret) > + return ret; > + > + cio2->csi2 = new V4L2Subdevice(entity); > + ret = cio2->csi2->open(); > + if (ret) > + return ret; > + > + std::string cio2Name = "ipu3-cio2 " + std::to_string(index); > + entity = cio2MediaDev_->getEntityByName(cio2Name); > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << cio2Name << "'"; > + return -EINVAL; > + } > + > + cio2->output = new V4L2Device(entity); > + ret = cio2->output->open(); Note for later, we shouldn't keep device nodes open when they're not needed. Depending on the kernel drivers, it may consume more power. Could you add a \todo ? > + if (ret) > + return ret; > + > + return 0; > +} > + > /* > * Cameras are created associating an image sensor (represented by a > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > @@ -329,85 +402,27 @@ error_release_mdev: > */ > void PipelineHandlerIPU3::registerCameras() > { > + int ret; > + > /* > * For each CSI-2 receiver on the IPU3, create a Camera if an > * image sensor is connected to it. > */ > 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; You could keep ret within the loop. > - > - /* > - * 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>(); > - > - std::string cameraName = sensor->name() + " " + std::to_string(id); > + std::unique_ptr<IPU3CameraData> data = > + utils::make_unique<IPU3CameraData>(); > std::vector<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; > No need for a blank line here. > - data->sensor_ = new V4L2Subdevice(sensor); > - ret = data->sensor_->open(); > + ret = initCio2(id, cio2); > if (ret) > 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); > > setCameraData(camera.get(), std::move(data)); > registerCamera(std::move(camera));
Hi Laurent, On Sat, Mar 02, 2019 at 11:04:44PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Feb 28, 2019 at 09:04:01PM +0100, Jacopo Mondi wrote: > > Group CIO2 devices (cio2, csi2 and image sensor) in a structure > > associated with the CameraData, to ease management of the CIO2 devices. > > > > Update the IPU3 pipeline handler implementation to avoid name clashes > > and break-out IPU3CameraData from the pipeline handler class for > > clarity. > > I'm not sure that really brings clarity :-) I would have kept the > IPU3CameraData structure where it was, but preferences differ of course. > The IPU3CameraData definition is not exported anyhow, so it would not make much difference if it is declared inside the IPU3PipelineHandler class. If you're not strongly against this, I would break it out. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 251 ++++++++++++++------------- > > 1 file changed, 133 insertions(+), 118 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 9694d0ce51ab..d3f1d9a95f81 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -24,6 +24,30 @@ namespace libcamera { > > > > LOG_DEFINE_CATEGORY(IPU3) > > > > +struct Cio2Device { > > Should this be called CIO2Device ? > ack > > + Cio2Device() > > + : output(nullptr), csi2(nullptr), sensor(nullptr) {} > > I'd open the {} on the next two lines for consistency, up to you. > ack > > + > > + ~Cio2Device() > > + { > > + delete output; > > + delete csi2; > > + delete sensor; > > + } > > I'm not very fond on having part of the logic in the PipelineHandlerIPU3 > class (the initCio2() function - I know I asked for it to be moved > there) part here in the destructor :-/ Splitting it in two doesn't feel > really right, I'd move these three delete statements to the > IPU3CameraData destructor. What do you think ? Re-looking at the series, I'm still in favor of aggregate all the CIO2-related components (csi2, sensor and output) in a structure, as an instance will always be associated with a Camera, and grouping them compacts the code a bit. I understand your concerns, but the CIO2 intialization routine would not just initialize components, but would actually make sure if a sensor is connected to the CIO2 instance, and it is used to decide if a camera has to be created or not. True we could make a class out the CIO2Device structure and add an init() method, whose result would be used to decide if to procede with creating the camera or not, but you where against this, and I would tend to agree as then would be tempting to put more and more methods in that class. To sum up, I agree with your first comment to not make a class out of this, that compared to the ImgUDevice one is trivally associated with a camera, and its components (sensor in particular) have to be interacted with by the IPU3PipelineHandler, but at the same time, I think it is nice to have a CIO2Device in the per-Camera IPU3CameraData, and centralize the component destruction in a single place. ImgUDevice is different, as we will assign them dynamically to Cameras, so they can't be destroyed when the CameraData is destroyed, but when the pipeline handler is. Ack for keeping this the way it is? > > > + > > + V4L2Device *output; > > + V4L2Subdevice *csi2; > > + V4L2Subdevice *sensor; > > +}; > > + > > +class IPU3CameraData : public CameraData > > +{ > > +public: > > + Cio2Device cio2; > > + > > + Stream stream_; > > +}; > > + > > class PipelineHandlerIPU3 : public PipelineHandler > > { > > public: > > @@ -47,65 +71,47 @@ public: > > bool match(DeviceEnumerator *enumerator); > > > > private: > > - class IPU3CameraData : public CameraData > > - { > > - public: > > - IPU3CameraData() > > - : cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {} > > - > > - ~IPU3CameraData() > > - { > > - delete cio2_; > > - delete csi2_; > > - delete sensor_; > > - } > > - > > - V4L2Device *cio2_; > > - V4L2Subdevice *csi2_; > > - V4L2Subdevice *sensor_; > > - > > - Stream stream_; > > - }; > > - > > IPU3CameraData *cameraData(const Camera *camera) > > { > > return static_cast<IPU3CameraData *>( > > PipelineHandler::cameraData(camera)); > > } > > > > + int initCio2(unsigned int index, Cio2Device *cio2); > > initCIO2() ? > > > void registerCameras(); > > > > - std::shared_ptr<MediaDevice> cio2_; > > - std::shared_ptr<MediaDevice> imgu_; > > + std::shared_ptr<MediaDevice> cio2MediaDev_; > > + std::shared_ptr<MediaDevice> imguMediaDev_; > > Are these renames really needed ? I have way too many CIO2s and ImgUs names around, clarify which one is the media device instance is helpfull. Got suggestions for better names? > > > }; > > > > 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> > > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > > std::vector<Stream *> &streams) > > { > > - IPU3CameraData *data = cameraData(camera); > > std::map<Stream *, StreamConfiguration> configs; > > + IPU3CameraData *data = cameraData(camera); > > + V4L2Subdevice *sensor = data->cio2.sensor; > > V4L2SubdeviceFormat format = {}; > > > > /* > > * FIXME: As of now, return the image format reported by the sensor. > > * In future good defaults should be provided for each stream. > > */ > > - if (data->sensor_->getFormat(0, &format)) { > > + if (sensor->getFormat(0, &format)) { > > LOG(IPU3, Error) << "Failed to create stream configurations"; > > return configs; > > } > > @@ -126,9 +132,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; > > @@ -185,13 +191,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; > > @@ -203,8 +210,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; > > @@ -216,9 +224,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > > int PipelineHandlerIPU3::start(const 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; > > @@ -230,14 +239,16 @@ int PipelineHandlerIPU3::start(const Camera *camera) > > void PipelineHandlerIPU3::stop(const 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(); > > } > > > > int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request) > > { > > IPU3CameraData *data = cameraData(camera); > > + V4L2Device *cio2 = data->cio2.output; > > Stream *stream = &data->stream_; > > > > Buffer *buffer = request->findBuffer(stream); > > @@ -247,7 +258,7 @@ int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request) > > return -ENOENT; > > } > > > > - data->cio2_->queueBuffer(buffer); > > + cio2->queueBuffer(buffer); > > > > return 0; > > } > > @@ -278,20 +289,20 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > imgu_dm.add("ipu3-imgu 1 viewfinder"); > > imgu_dm.add("ipu3-imgu 1 3a stat"); > > > > - cio2_ = enumerator->search(cio2_dm); > > - if (!cio2_) > > + cio2MediaDev_ = enumerator->search(cio2_dm); > > + if (!cio2MediaDev_) > > return false; > > > > - imgu_ = enumerator->search(imgu_dm); > > - if (!imgu_) > > + imguMediaDev_ = enumerator->search(imgu_dm); > > + if (!imguMediaDev_) > > return false; > > > > /* > > * It is safe to acquire both media devices at this point as > > * DeviceEnumerator::search() skips the busy ones for us. > > */ > > - cio2_->acquire(); > > - imgu_->acquire(); > > + cio2MediaDev_->acquire(); > > + imguMediaDev_->acquire(); > > > > /* > > * Disable all links that are enabled by default on CIO2, as camera > > @@ -300,28 +311,90 @@ 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()) > > goto error_release_mdev; > > > > - if (cio2_->disableLinks()) > > + if (cio2MediaDev_->disableLinks()) > > goto error_close_cio2; > > > > registerCameras(); > > > > - cio2_->close(); > > + cio2MediaDev_->close(); > > > > return true; > > > > error_close_cio2: > > - cio2_->close(); > > + cio2MediaDev_->close(); > > > > error_release_mdev: > > - cio2_->release(); > > - imgu_->release(); > > + cio2MediaDev_->release(); > > + imguMediaDev_->release(); > > > > return false; > > } > > > > This pipeline handler will likely be used as a reference implementation > in the future, could you add more documentation, even for private > functions ? > Yes, will use doxygen style (minor thing: doxygen won't produce documentation for private memebers but should I use the /** comment block starter anyway for consistency ? ) > > +int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > > +{ > > + int ret; > > + > > + /* Verify a sensor subdevice is connected to this CIO2 instance. */ > > + std::string csi2Name = "ipu3-csi2 " + std::to_string(index); > > + MediaEntity *entity = cio2MediaDev_->getEntityByName(csi2Name); > > + if (!entity) { > > + LOG(IPU3, Error) > > + << "Failed to get entity '" << csi2Name << "'"; > > + return -ENODEV; > > + } > > + > > + 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, create and open > > + * video devices and subdevices associated with this CIO2 unit. > > + */ > > + cio2->sensor = new V4L2Subdevice(sensorEntity); > > + ret = cio2->sensor->open(); > > + if (ret) > > + return ret; > > + > > + cio2->csi2 = new V4L2Subdevice(entity); > > + ret = cio2->csi2->open(); > > + if (ret) > > + return ret; > > + > > + std::string cio2Name = "ipu3-cio2 " + std::to_string(index); > > + entity = cio2MediaDev_->getEntityByName(cio2Name); > > + if (!entity) { > > + LOG(IPU3, Error) > > + << "Failed to get entity '" << cio2Name << "'"; > > + return -EINVAL; > > + } > > + > > + cio2->output = new V4L2Device(entity); > > + ret = cio2->output->open(); > > Note for later, we shouldn't keep device nodes open when they're not > needed. Depending on the kernel drivers, it may consume more power. > Could you add a \todo ? > Yes indeed. > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > /* > > * Cameras are created associating an image sensor (represented by a > > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > @@ -329,85 +402,27 @@ error_release_mdev: > > */ > > void PipelineHandlerIPU3::registerCameras() > > { > > + int ret; > > + > > /* > > * For each CSI-2 receiver on the IPU3, create a Camera if an > > * image sensor is connected to it. > > */ > > 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; > > You could keep ret within the loop. > ack > > - > > - /* > > - * 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>(); > > - > > - std::string cameraName = sensor->name() + " " + std::to_string(id); > > + std::unique_ptr<IPU3CameraData> data = > > + utils::make_unique<IPU3CameraData>(); > > std::vector<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; > > > > No need for a blank line here. > I'll fix. Thanks j > > - data->sensor_ = new V4L2Subdevice(sensor); > > - ret = data->sensor_->open(); > > + ret = initCio2(id, cio2); > > if (ret) > > 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); > > > > setCameraData(camera.get(), std::move(data)); > > registerCamera(std::move(camera)); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 9694d0ce51ab..d3f1d9a95f81 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -24,6 +24,30 @@ 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 IPU3CameraData : public CameraData +{ +public: + Cio2Device cio2; + + Stream stream_; +}; + class PipelineHandlerIPU3 : public PipelineHandler { public: @@ -47,65 +71,47 @@ public: bool match(DeviceEnumerator *enumerator); private: - class IPU3CameraData : public CameraData - { - public: - IPU3CameraData() - : cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {} - - ~IPU3CameraData() - { - delete cio2_; - delete csi2_; - delete sensor_; - } - - V4L2Device *cio2_; - V4L2Subdevice *csi2_; - V4L2Subdevice *sensor_; - - Stream stream_; - }; - IPU3CameraData *cameraData(const Camera *camera) { return static_cast<IPU3CameraData *>( PipelineHandler::cameraData(camera)); } + 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> PipelineHandlerIPU3::streamConfiguration(Camera *camera, std::vector<Stream *> &streams) { - IPU3CameraData *data = cameraData(camera); std::map<Stream *, StreamConfiguration> configs; + IPU3CameraData *data = cameraData(camera); + V4L2Subdevice *sensor = data->cio2.sensor; V4L2SubdeviceFormat format = {}; /* * FIXME: As of now, return the image format reported by the sensor. * In future good defaults should be provided for each stream. */ - if (data->sensor_->getFormat(0, &format)) { + if (sensor->getFormat(0, &format)) { LOG(IPU3, Error) << "Failed to create stream configurations"; return configs; } @@ -126,9 +132,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; @@ -185,13 +191,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; @@ -203,8 +210,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; @@ -216,9 +224,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) int PipelineHandlerIPU3::start(const 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; @@ -230,14 +239,16 @@ int PipelineHandlerIPU3::start(const Camera *camera) void PipelineHandlerIPU3::stop(const 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(); } int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request) { IPU3CameraData *data = cameraData(camera); + V4L2Device *cio2 = data->cio2.output; Stream *stream = &data->stream_; Buffer *buffer = request->findBuffer(stream); @@ -247,7 +258,7 @@ int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request) return -ENOENT; } - data->cio2_->queueBuffer(buffer); + cio2->queueBuffer(buffer); return 0; } @@ -278,20 +289,20 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) imgu_dm.add("ipu3-imgu 1 viewfinder"); imgu_dm.add("ipu3-imgu 1 3a stat"); - cio2_ = enumerator->search(cio2_dm); - if (!cio2_) + cio2MediaDev_ = enumerator->search(cio2_dm); + if (!cio2MediaDev_) return false; - imgu_ = enumerator->search(imgu_dm); - if (!imgu_) + imguMediaDev_ = enumerator->search(imgu_dm); + if (!imguMediaDev_) return false; /* * It is safe to acquire both media devices at this point as * DeviceEnumerator::search() skips the busy ones for us. */ - cio2_->acquire(); - imgu_->acquire(); + cio2MediaDev_->acquire(); + imguMediaDev_->acquire(); /* * Disable all links that are enabled by default on CIO2, as camera @@ -300,28 +311,90 @@ 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()) goto error_release_mdev; - if (cio2_->disableLinks()) + if (cio2MediaDev_->disableLinks()) goto error_close_cio2; registerCameras(); - cio2_->close(); + cio2MediaDev_->close(); return true; error_close_cio2: - cio2_->close(); + cio2MediaDev_->close(); error_release_mdev: - cio2_->release(); - imgu_->release(); + cio2MediaDev_->release(); + imguMediaDev_->release(); return false; } +int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) +{ + int ret; + + /* Verify a sensor subdevice is connected to this CIO2 instance. */ + std::string csi2Name = "ipu3-csi2 " + std::to_string(index); + MediaEntity *entity = cio2MediaDev_->getEntityByName(csi2Name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << csi2Name << "'"; + return -ENODEV; + } + + 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, create and open + * video devices and subdevices associated with this CIO2 unit. + */ + cio2->sensor = new V4L2Subdevice(sensorEntity); + ret = cio2->sensor->open(); + if (ret) + return ret; + + cio2->csi2 = new V4L2Subdevice(entity); + ret = cio2->csi2->open(); + if (ret) + return ret; + + std::string cio2Name = "ipu3-cio2 " + std::to_string(index); + entity = cio2MediaDev_->getEntityByName(cio2Name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << cio2Name << "'"; + return -EINVAL; + } + + cio2->output = new V4L2Device(entity); + ret = cio2->output->open(); + if (ret) + return ret; + + return 0; +} + /* * Cameras are created associating an image sensor (represented by a * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four @@ -329,85 +402,27 @@ error_release_mdev: */ void PipelineHandlerIPU3::registerCameras() { + int ret; + /* * For each CSI-2 receiver on the IPU3, create a Camera if an * image sensor is connected to it. */ 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>(); - - std::string cameraName = sensor->name() + " " + std::to_string(id); + std::unique_ptr<IPU3CameraData> data = + utils::make_unique<IPU3CameraData>(); std::vector<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; - data->sensor_ = new V4L2Subdevice(sensor); - ret = data->sensor_->open(); + ret = initCio2(id, cio2); if (ret) 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); setCameraData(camera.get(), std::move(data)); registerCamera(std::move(camera));
Group CIO2 devices (cio2, csi2 and image sensor) in a structure associated with the CameraData, to ease management of the CIO2 devices. Update the IPU3 pipeline handler implementation to avoid name clashes and break-out IPU3CameraData from the pipeline handler class for clarity. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 251 ++++++++++++++------------- 1 file changed, 133 insertions(+), 118 deletions(-)