| Message ID | 20190320163055.22056-7-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:30PM +0100, Jacopo Mondi wrote: > Create video devices and subdevices associated with an ImgU unit at > camera registration time. > > Statically assign imgu0 to the first camera and imgu1 to the second one > and limit support to two camera. This will have to be revised in future. s/camera/cameras/ s/in future/in the future/ > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 223 +++++++++++++++++++++++++-- > 1 file changed, 209 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2e351d04a784..26fc56a76eb1 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -27,6 +27,38 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +class ImgUDevice > +{ > +public: > + ImgUDevice() > + : imgu(nullptr), input(nullptr), output(nullptr), > + viewfinder(nullptr), stat(nullptr) > + { > + } > + > + ~ImgUDevice() > + { > + delete imgu; > + delete input; > + delete output; > + delete viewfinder; > + delete stat; > + } > + > + void init(MediaDevice *media, unsigned int index); > + > + unsigned int index_; > + std::string imguName_; As this is the ImgUDevice class, maybe just "name_" ? > + MediaDevice *mediaDevice_; > + > + V4L2Subdevice *imgu; > + V4L2Device *input; > + V4L2Device *output; > + V4L2Device *viewfinder; > + V4L2Device *stat; Missing _ at the end of each variable. > + /* \todo Add param video device for 3A tuning */ > +}; > + > struct CIO2Device { > CIO2Device() > : output(nullptr), csi2(nullptr), sensor(nullptr) > @@ -68,6 +100,7 @@ public: > bool match(DeviceEnumerator *enumerator); > > private: > + static constexpr unsigned int IPU3_IMGU_COUNT = 2; > static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > > class IPU3CameraData : public CameraData > @@ -81,6 +114,7 @@ private: > void bufferReady(Buffer *buffer); > > CIO2Device cio2; > + ImgUDevice *imgu; Here too. > > Stream stream_; > }; > @@ -92,10 +126,18 @@ private: > } > > int mediaBusToCIO2Format(unsigned int code); > + V4L2Device *openDevice(MediaDevice *media, const std::string &name); > + V4L2Subdevice *openSubdevice(MediaDevice *media, > + const std::string &name); > > int initCIO2(unsigned int index, CIO2Device *cio2); > + void deleteCIO2(CIO2Device *cio2); > + > + int initImgU(ImgUDevice *imgu); > + > void registerCameras(); > > + ImgUDevice imgus_[IPU3_IMGU_COUNT]; > std::shared_ptr<MediaDevice> cio2MediaDev_; > std::shared_ptr<MediaDevice> imguMediaDev_; > }; > @@ -355,11 +397,45 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > return false; > } > > + if (imguMediaDev_->open()) { > + cio2MediaDev_->close(); > + return false; > + } > + > + if (imguMediaDev_->disableLinks()) > + goto error_close_mdev; > + > + for (unsigned int i = 0; i < IPU3_IMGU_COUNT; ++i) > + imgus_[i].init(imguMediaDev_.get(), i); > + > registerCameras(); > > cio2MediaDev_->close(); > + imguMediaDev_->close(); > > return true; > + > +error_close_mdev: There's a single error label, just call it error. > + cio2MediaDev_->close(); > + imguMediaDev_->close(); > + > + return false; > +} > + > +/* ---------------------------------------------------------------------------- > + * Helpers > + */ > + > +/** > + * \brief Initialize fields of the ImgU instance > + * \param mediaDevice The ImgU instance media device > + * \param index The ImgU instance index > + */ > +void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index) > +{ > + index_ = index; > + imguName_ = "ipu3-imgu " + std::to_string(index_); > + mediaDevice_ = mediaDevice; > } > > int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) > @@ -378,6 +454,114 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) > } > } > > +/** > + * \brief Create and open the video device with \a name in media device \a media > + * > + * \todo Make a generic helper out of this method. Please do :-) You can create a static function in the V4L2Device class that takes a media device and entity name and return a newly created V4L2Device. I would however keep the open() call outside of the helper as we'll need to refactor open/close later. > + * > + * \return Pointer to the video device on success, nullptr otherwise > + */ > +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, > + const std::string &name) > +{ > + MediaEntity *entity = media->getEntityByName(name); > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << name << "'"; > + return nullptr; > + } > + > + V4L2Device *dev = new V4L2Device(entity); > + if (dev->open()) { > + delete dev; > + return nullptr; > + } > + > + return dev; > +} > + > +/** > + * \brief Create and open the subdevice with \a name in media device \a media > + * > + * \todo Make a generic helper out of this method. I'd say the same here, but you use this helper in a single location, so I would just inline it. > + * > + * \return Pointer to the subdevice on success, nullptr otherwise > + */ > +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, > + const std::string &name) > +{ > + MediaEntity *entity = media->getEntityByName(name); > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << name << "'"; > + return nullptr; > + } > + > + V4L2Subdevice *dev = new V4L2Subdevice(entity); > + if (dev->open()) { > + delete dev; > + return nullptr; > + } > + > + return dev; > +} > + > +/* ---------------------------------------------------------------------------- > + * IPU3 pipeline configuration > + */ > + > +/** > + * \brief Initialize and configure components of the ImgU instance > + * > + * Create and open the devices and subdevices in the ImgU instance. > + * This methods configures the ImgU instance for capture operations, and > + * should be called at stream configuration time. > + * > + * \todo Expand the ImgU configuration with controls setting > + * > + * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV Failed to open one of the video devices or subdevices > + */ > +int PipelineHandlerIPU3::initImgU(ImgUDevice *imgu) > +{ > + imgu->imgu = openSubdevice(imgu->mediaDevice_, imgu->imguName_); > + if (!imgu->imgu) > + return -ENODEV; > + > + imgu->input = openDevice(imgu->mediaDevice_, > + imgu->imguName_ + " input"); > + if (!imgu->input) > + goto error_delete_imgu; > + > + imgu->output = openDevice(imgu->mediaDevice_, > + imgu->imguName_ + " output"); > + if (!imgu->output) > + goto error_delete_input; > + > + imgu->viewfinder = openDevice(imgu->mediaDevice_, > + imgu->imguName_ + " viewfinder"); > + if (!imgu->viewfinder) > + goto error_delete_output; > + > + imgu->stat = openDevice(imgu->mediaDevice_, > + imgu->imguName_ + " 3a stat"); > + if (!imgu->stat) > + goto error_delete_vf; > + > + return 0; > + > +error_delete_vf: > + delete imgu->viewfinder; > +error_delete_output: > + delete imgu->output; > +error_delete_input: > + delete imgu->input; > +error_delete_imgu: > + delete imgu->imgu; Let's simplify this by removing the need for this code. The caller can just propagate the error up, the match function will fail, and the imgu instances will be destroyed. > + > + return -ENODEV; > +} > + > /** > * \brief Initialize components of the CIO2 device \a index used by a camera > * \param index The CIO2 device index > @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2) > if (ret) > goto error_delete_csi2; > > - entity = cio2MediaDev_->getEntityByName(cio2Name); > - if (!entity) { > - LOG(IPU3, Error) > - << "Failed to get entity '" << cio2Name << "'"; > - ret = -EINVAL; > + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > + if (!cio2->output) > 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: > @@ -483,6 +656,16 @@ error_delete_sensor: > return ret; > } > > +/** > + * \brief Delete all devices associated with a CIO2 unit > + */ > +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) > +{ > + delete cio2->output; > + delete cio2->csi2; > + delete cio2->sensor; > +} I don't think this is needed for the reason explained above. > + > /* > * Cameras are created associating an image sensor (represented by a > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras() > * image sensor is connected to it. > */ > unsigned int numCameras = 0; > - for (unsigned int id = 0; id < 4; ++id) { > + for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) { > std::unique_ptr<IPU3CameraData> data = > utils::make_unique<IPU3CameraData>(this); > std::set<Stream *> streams{ &data->stream_ }; > @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras() > if (ret) > continue; > > + /** > + * \todo Dynamically assign ImgU devices; as of now, limit > + * support to two cameras only, and assign imgu0 to the first > + * one and imgu1 to the second. > + */ > + data->imgu = &imgus_[numCameras]; > + ret = initImgU(data->imgu); The imgus are separate from the cameras, please initialize them before this loop, and return an error if initialization fails. > + if (ret) { > + deleteCIO2(cio2); > + continue; > + } > + > std::string cameraName = cio2->sensor->deviceName() + " " > + std::to_string(id); > std::shared_ptr<Camera> camera = Camera::create(this,
Hi Laurent, On Thu, Mar 21, 2019 at 11:33:25AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Mar 20, 2019 at 05:30:30PM +0100, Jacopo Mondi wrote: > > Create video devices and subdevices associated with an ImgU unit at > > camera registration time. > > > > Statically assign imgu0 to the first camera and imgu1 to the second one > > and limit support to two camera. This will have to be revised in future. > > s/camera/cameras/ > s/in future/in the future/ > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 223 +++++++++++++++++++++++++-- > > 1 file changed, 209 insertions(+), 14 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 2e351d04a784..26fc56a76eb1 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -27,6 +27,38 @@ namespace libcamera { > > > > LOG_DEFINE_CATEGORY(IPU3) > > > > +class ImgUDevice > > +{ > > +public: > > + ImgUDevice() > > + : imgu(nullptr), input(nullptr), output(nullptr), > > + viewfinder(nullptr), stat(nullptr) > > + { > > + } > > + > > + ~ImgUDevice() > > + { > > + delete imgu; > > + delete input; > > + delete output; > > + delete viewfinder; > > + delete stat; > > + } > > + > > + void init(MediaDevice *media, unsigned int index); > > + > > + unsigned int index_; > > + std::string imguName_; > > As this is the ImgUDevice class, maybe just "name_" ? > > > + MediaDevice *mediaDevice_; > > + > > + V4L2Subdevice *imgu; > > + V4L2Device *input; > > + V4L2Device *output; > > + V4L2Device *viewfinder; > > + V4L2Device *stat; > > Missing _ at the end of each variable. > > > + /* \todo Add param video device for 3A tuning */ > > +}; > > + > > struct CIO2Device { > > CIO2Device() > > : output(nullptr), csi2(nullptr), sensor(nullptr) > > @@ -68,6 +100,7 @@ public: > > bool match(DeviceEnumerator *enumerator); > > > > private: > > + static constexpr unsigned int IPU3_IMGU_COUNT = 2; > > static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > > > > class IPU3CameraData : public CameraData > > @@ -81,6 +114,7 @@ private: > > void bufferReady(Buffer *buffer); > > > > CIO2Device cio2; > > + ImgUDevice *imgu; > > Here too. > > > > > Stream stream_; > > }; > > @@ -92,10 +126,18 @@ private: > > } > > > > int mediaBusToCIO2Format(unsigned int code); > > + V4L2Device *openDevice(MediaDevice *media, const std::string &name); > > + V4L2Subdevice *openSubdevice(MediaDevice *media, > > + const std::string &name); > > > > int initCIO2(unsigned int index, CIO2Device *cio2); > > + void deleteCIO2(CIO2Device *cio2); > > + > > + int initImgU(ImgUDevice *imgu); > > + > > void registerCameras(); > > > > + ImgUDevice imgus_[IPU3_IMGU_COUNT]; > > std::shared_ptr<MediaDevice> cio2MediaDev_; > > std::shared_ptr<MediaDevice> imguMediaDev_; > > }; > > @@ -355,11 +397,45 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > return false; > > } > > > > + if (imguMediaDev_->open()) { > > + cio2MediaDev_->close(); > > + return false; > > + } > > + > > + if (imguMediaDev_->disableLinks()) > > + goto error_close_mdev; > > + > > + for (unsigned int i = 0; i < IPU3_IMGU_COUNT; ++i) > > + imgus_[i].init(imguMediaDev_.get(), i); > > + > > registerCameras(); > > > > cio2MediaDev_->close(); > > + imguMediaDev_->close(); > > > > return true; > > + > > +error_close_mdev: > > There's a single error label, just call it error. > > > + cio2MediaDev_->close(); > > + imguMediaDev_->close(); > > + > > + return false; > > +} > > + > > +/* ---------------------------------------------------------------------------- > > + * Helpers > > + */ > > + > > +/** > > + * \brief Initialize fields of the ImgU instance > > + * \param mediaDevice The ImgU instance media device > > + * \param index The ImgU instance index > > + */ > > +void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index) > > +{ > > + index_ = index; > > + imguName_ = "ipu3-imgu " + std::to_string(index_); > > + mediaDevice_ = mediaDevice; > > } > > > > int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) > > @@ -378,6 +454,114 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) > > } > > } > > > > +/** > > + * \brief Create and open the video device with \a name in media device \a media > > + * > > + * \todo Make a generic helper out of this method. > > Please do :-) You can create a static function in the V4L2Device class > that takes a media device and entity name and return a newly created > V4L2Device. I would however keep the open() call outside of the helper > as we'll need to refactor open/close later. > > > + * > > + * \return Pointer to the video device on success, nullptr otherwise > > + */ > > +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, > > + const std::string &name) > > +{ > > + MediaEntity *entity = media->getEntityByName(name); > > + if (!entity) { > > + LOG(IPU3, Error) > > + << "Failed to get entity '" << name << "'"; > > + return nullptr; > > + } > > + > > + V4L2Device *dev = new V4L2Device(entity); > > + if (dev->open()) { > > + delete dev; > > + return nullptr; > > + } > > + > > + return dev; > > +} > > + > > +/** > > + * \brief Create and open the subdevice with \a name in media device \a media > > + * > > + * \todo Make a generic helper out of this method. > > I'd say the same here, but you use this helper in a single location, so > I would just inline it. > > > + * > > + * \return Pointer to the subdevice on success, nullptr otherwise > > + */ > > +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, > > + const std::string &name) > > +{ > > + MediaEntity *entity = media->getEntityByName(name); > > + if (!entity) { > > + LOG(IPU3, Error) > > + << "Failed to get entity '" << name << "'"; > > + return nullptr; > > + } > > + > > + V4L2Subdevice *dev = new V4L2Subdevice(entity); > > + if (dev->open()) { > > + delete dev; > > + return nullptr; > > + } > > + > > + return dev; > > +} > > + > > +/* ---------------------------------------------------------------------------- > > + * IPU3 pipeline configuration > > + */ > > + > > +/** > > + * \brief Initialize and configure components of the ImgU instance > > + * > > + * Create and open the devices and subdevices in the ImgU instance. > > + * This methods configures the ImgU instance for capture operations, and > > + * should be called at stream configuration time. > > + * > > + * \todo Expand the ImgU configuration with controls setting > > + * > > + * \return 0 on success or a negative error code otherwise > > + * \retval -ENODEV Failed to open one of the video devices or subdevices > > + */ > > +int PipelineHandlerIPU3::initImgU(ImgUDevice *imgu) > > +{ > > + imgu->imgu = openSubdevice(imgu->mediaDevice_, imgu->imguName_); > > + if (!imgu->imgu) > > + return -ENODEV; > > + > > + imgu->input = openDevice(imgu->mediaDevice_, > > + imgu->imguName_ + " input"); > > + if (!imgu->input) > > + goto error_delete_imgu; > > + > > + imgu->output = openDevice(imgu->mediaDevice_, > > + imgu->imguName_ + " output"); > > + if (!imgu->output) > > + goto error_delete_input; > > + > > + imgu->viewfinder = openDevice(imgu->mediaDevice_, > > + imgu->imguName_ + " viewfinder"); > > + if (!imgu->viewfinder) > > + goto error_delete_output; > > + > > + imgu->stat = openDevice(imgu->mediaDevice_, > > + imgu->imguName_ + " 3a stat"); > > + if (!imgu->stat) > > + goto error_delete_vf; > > + > > + return 0; > > + > > +error_delete_vf: > > + delete imgu->viewfinder; > > +error_delete_output: > > + delete imgu->output; > > +error_delete_input: > > + delete imgu->input; > > +error_delete_imgu: > > + delete imgu->imgu; > > Let's simplify this by removing the need for this code. The caller can > just propagate the error up, the match function will fail, and the imgu > instances will be destroyed. > From your comment I get that we can fail when intializing the ImgUs, propagate the error up and make PipelineHandler::match() fail, which deletes the pipeline handler instance and consequentially the ImgU instances. Just to make sure we're on the same page: - the pipeline manager instances are destroyed at CameraManager::stop() time only - if PipelineHandler::match() fails, the pipeline handler is not destroyed. - It is not enough to return an error from the PipelineHandler::match() function to have the ImgU initialized instances (if any) deleted It is very likely we can wait for CameraManager::stop() and have the ImgU instances destroyed there, but I wonder if it would not be worth cleaning them up as soon as we fail. The easies way would be instanciate them at initialization time, and delete the newly created instances if one of the two initializations fail, or associate them with camera data as we do for CIO2Device instances, but then your comment to initialize ImgUs separately from camera creation won't apply anymore. My proposal would be something like: class ImgUDevice { ... ImgUDevice *imgu0_; ImgUDevice *imgu1_; } PipelineHandler::match() { ret = registerCameras(); if (ret) { closeMediaDevices return false; } } PipelineHandler::registerCameras() { imgu0_ = new ImgUDevice(); ret = imgu0_->init(); if (ret) { delete imgu0_; return -ENODEV; } imgu1_ = new ImgUDevice(); ret = imgu1_->init(); if (ret) { delete imgu0_; delete imgu1_; return -ENODEV; } for (i = 0 ; i < 4; ++i) { ret = cio2::init(); if (ret) continue; registerCamera(); ++numCameras; } if (!numCameras) { delete imgu0_; delete imgu1_; return -ENODEV; } return 0; } Opinions? Thanks j > > + > > + return -ENODEV; > > +} > > + > > /** > > * \brief Initialize components of the CIO2 device \a index used by a camera > > * \param index The CIO2 device index > > @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2) > > if (ret) > > goto error_delete_csi2; > > > > - entity = cio2MediaDev_->getEntityByName(cio2Name); > > - if (!entity) { > > - LOG(IPU3, Error) > > - << "Failed to get entity '" << cio2Name << "'"; > > - ret = -EINVAL; > > + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > > + if (!cio2->output) > > 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: > > @@ -483,6 +656,16 @@ error_delete_sensor: > > return ret; > > } > > > > +/** > > + * \brief Delete all devices associated with a CIO2 unit > > + */ > > +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) > > +{ > > + delete cio2->output; > > + delete cio2->csi2; > > + delete cio2->sensor; > > +} > > I don't think this is needed for the reason explained above. > > > + > > /* > > * Cameras are created associating an image sensor (represented by a > > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras() > > * image sensor is connected to it. > > */ > > unsigned int numCameras = 0; > > - for (unsigned int id = 0; id < 4; ++id) { > > + for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) { > > std::unique_ptr<IPU3CameraData> data = > > utils::make_unique<IPU3CameraData>(this); > > std::set<Stream *> streams{ &data->stream_ }; > > @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras() > > if (ret) > > continue; > > > > + /** > > + * \todo Dynamically assign ImgU devices; as of now, limit > > + * support to two cameras only, and assign imgu0 to the first > > + * one and imgu1 to the second. > > + */ > > + data->imgu = &imgus_[numCameras]; > > + ret = initImgU(data->imgu); > > The imgus are separate from the cameras, please initialize them before > this loop, and return an error if initialization fails. > > > + if (ret) { > > + deleteCIO2(cio2); > > + continue; > > + } > > + > > std::string cameraName = cio2->sensor->deviceName() + " " > > + std::to_string(id); > > std::shared_ptr<Camera> camera = Camera::create(this, > > -- > Regards, > > Laurent Pinchart
Ups, self correction... On Mon, Mar 25, 2019 at 10:43:06AM +0100, Jacopo Mondi wrote: > Hi Laurent, > [snip] > > > + return 0; > > > + > > > +error_delete_vf: > > > + delete imgu->viewfinder; > > > +error_delete_output: > > > + delete imgu->output; > > > +error_delete_input: > > > + delete imgu->input; > > > +error_delete_imgu: > > > + delete imgu->imgu; > > > > Let's simplify this by removing the need for this code. The caller can > > just propagate the error up, the match function will fail, and the imgu > > instances will be destroyed. > > > > From your comment I get that we can fail when intializing the ImgUs, > propagate the error up and make PipelineHandler::match() fail, which > deletes the pipeline handler instance and consequentially the ImgU > instances. > > Just to make sure we're on the same page: > - the pipeline manager instances are destroyed at > CameraManager::stop() time only > - if PipelineHandler::match() fails, the pipeline handler is not > destroyed. It is, as it is created as shared pointer with a single reference by PipelineHandlerFactory::create(). Sorry for the noise, I had missed that the one created at match() time is the single reference to the newly created pipeline handler. Thanks for pointing me to that. > - It is not enough to return an error from the > PipelineHandler::match() function to have the ImgU initialized instances > (if any) deleted > > It is very likely we can wait for CameraManager::stop() and have the > ImgU instances destroyed there, but I wonder if it would not be worth > cleaning them up as soon as we fail. The easies way would be > instanciate them at initialization time, and delete the newly created > instances if one of the two initializations fail, or associate them > with camera data as we do for CIO2Device instances, but then your > comment to initialize ImgUs separately from camera creation won't > apply anymore. > > My proposal would be something like: > > class ImgUDevice > { > ... > > ImgUDevice *imgu0_; > ImgUDevice *imgu1_; > } > > PipelineHandler::match() > { > > > ret = registerCameras(); > if (ret) { > closeMediaDevices > return false; > } > } > > PipelineHandler::registerCameras() > { > > imgu0_ = new ImgUDevice(); > ret = imgu0_->init(); > if (ret) { > delete imgu0_; > return -ENODEV; > } > > imgu1_ = new ImgUDevice(); > ret = imgu1_->init(); > if (ret) { > delete imgu0_; > delete imgu1_; > return -ENODEV; > } > > for (i = 0 ; i < 4; ++i) { > ret = cio2::init(); > if (ret) > continue; > > registerCamera(); > ++numCameras; > } > > if (!numCameras) { > delete imgu0_; > delete imgu1_; > > return -ENODEV; > } > > return 0; > } > > Opinions? > > Thanks > j > > > > + > > > + return -ENODEV; > > > +} > > > + > > > /** > > > * \brief Initialize components of the CIO2 device \a index used by a camera > > > * \param index The CIO2 device index > > > @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2) > > > if (ret) > > > goto error_delete_csi2; > > > > > > - entity = cio2MediaDev_->getEntityByName(cio2Name); > > > - if (!entity) { > > > - LOG(IPU3, Error) > > > - << "Failed to get entity '" << cio2Name << "'"; > > > - ret = -EINVAL; > > > + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > > > + if (!cio2->output) > > > 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: > > > @@ -483,6 +656,16 @@ error_delete_sensor: > > > return ret; > > > } > > > > > > +/** > > > + * \brief Delete all devices associated with a CIO2 unit > > > + */ > > > +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) > > > +{ > > > + delete cio2->output; > > > + delete cio2->csi2; > > > + delete cio2->sensor; > > > +} > > > > I don't think this is needed for the reason explained above. > > > > > + > > > /* > > > * Cameras are created associating an image sensor (represented by a > > > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > > @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras() > > > * image sensor is connected to it. > > > */ > > > unsigned int numCameras = 0; > > > - for (unsigned int id = 0; id < 4; ++id) { > > > + for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) { > > > std::unique_ptr<IPU3CameraData> data = > > > utils::make_unique<IPU3CameraData>(this); > > > std::set<Stream *> streams{ &data->stream_ }; > > > @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras() > > > if (ret) > > > continue; > > > > > > + /** > > > + * \todo Dynamically assign ImgU devices; as of now, limit > > > + * support to two cameras only, and assign imgu0 to the first > > > + * one and imgu1 to the second. > > > + */ > > > + data->imgu = &imgus_[numCameras]; > > > + ret = initImgU(data->imgu); > > > > The imgus are separate from the cameras, please initialize them before > > this loop, and return an error if initialization fails. > > > > > + if (ret) { > > > + deleteCIO2(cio2); > > > + continue; > > > + } > > > + > > > std::string cameraName = cio2->sensor->deviceName() + " " > > > + std::to_string(id); > > > std::shared_ptr<Camera> camera = Camera::create(this, > > > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 2e351d04a784..26fc56a76eb1 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -27,6 +27,38 @@ namespace libcamera { LOG_DEFINE_CATEGORY(IPU3) +class ImgUDevice +{ +public: + ImgUDevice() + : imgu(nullptr), input(nullptr), output(nullptr), + viewfinder(nullptr), stat(nullptr) + { + } + + ~ImgUDevice() + { + delete imgu; + delete input; + delete output; + delete viewfinder; + delete stat; + } + + void init(MediaDevice *media, unsigned int index); + + unsigned int index_; + std::string imguName_; + MediaDevice *mediaDevice_; + + V4L2Subdevice *imgu; + V4L2Device *input; + V4L2Device *output; + V4L2Device *viewfinder; + V4L2Device *stat; + /* \todo Add param video device for 3A tuning */ +}; + struct CIO2Device { CIO2Device() : output(nullptr), csi2(nullptr), sensor(nullptr) @@ -68,6 +100,7 @@ public: bool match(DeviceEnumerator *enumerator); private: + static constexpr unsigned int IPU3_IMGU_COUNT = 2; static constexpr unsigned int IPU3_BUFFER_COUNT = 4; class IPU3CameraData : public CameraData @@ -81,6 +114,7 @@ private: void bufferReady(Buffer *buffer); CIO2Device cio2; + ImgUDevice *imgu; Stream stream_; }; @@ -92,10 +126,18 @@ private: } int mediaBusToCIO2Format(unsigned int code); + V4L2Device *openDevice(MediaDevice *media, const std::string &name); + V4L2Subdevice *openSubdevice(MediaDevice *media, + const std::string &name); int initCIO2(unsigned int index, CIO2Device *cio2); + void deleteCIO2(CIO2Device *cio2); + + int initImgU(ImgUDevice *imgu); + void registerCameras(); + ImgUDevice imgus_[IPU3_IMGU_COUNT]; std::shared_ptr<MediaDevice> cio2MediaDev_; std::shared_ptr<MediaDevice> imguMediaDev_; }; @@ -355,11 +397,45 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) return false; } + if (imguMediaDev_->open()) { + cio2MediaDev_->close(); + return false; + } + + if (imguMediaDev_->disableLinks()) + goto error_close_mdev; + + for (unsigned int i = 0; i < IPU3_IMGU_COUNT; ++i) + imgus_[i].init(imguMediaDev_.get(), i); + registerCameras(); cio2MediaDev_->close(); + imguMediaDev_->close(); return true; + +error_close_mdev: + cio2MediaDev_->close(); + imguMediaDev_->close(); + + return false; +} + +/* ---------------------------------------------------------------------------- + * Helpers + */ + +/** + * \brief Initialize fields of the ImgU instance + * \param mediaDevice The ImgU instance media device + * \param index The ImgU instance index + */ +void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index) +{ + index_ = index; + imguName_ = "ipu3-imgu " + std::to_string(index_); + mediaDevice_ = mediaDevice; } int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) @@ -378,6 +454,114 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code) } } +/** + * \brief Create and open the video device with \a name in media device \a media + * + * \todo Make a generic helper out of this method. + * + * \return Pointer to the video device on success, nullptr otherwise + */ +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, + const std::string &name) +{ + MediaEntity *entity = media->getEntityByName(name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << name << "'"; + return nullptr; + } + + V4L2Device *dev = new V4L2Device(entity); + if (dev->open()) { + delete dev; + return nullptr; + } + + return dev; +} + +/** + * \brief Create and open the subdevice with \a name in media device \a media + * + * \todo Make a generic helper out of this method. + * + * \return Pointer to the subdevice on success, nullptr otherwise + */ +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, + const std::string &name) +{ + MediaEntity *entity = media->getEntityByName(name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << name << "'"; + return nullptr; + } + + V4L2Subdevice *dev = new V4L2Subdevice(entity); + if (dev->open()) { + delete dev; + return nullptr; + } + + return dev; +} + +/* ---------------------------------------------------------------------------- + * IPU3 pipeline configuration + */ + +/** + * \brief Initialize and configure components of the ImgU instance + * + * Create and open the devices and subdevices in the ImgU instance. + * This methods configures the ImgU instance for capture operations, and + * should be called at stream configuration time. + * + * \todo Expand the ImgU configuration with controls setting + * + * \return 0 on success or a negative error code otherwise + * \retval -ENODEV Failed to open one of the video devices or subdevices + */ +int PipelineHandlerIPU3::initImgU(ImgUDevice *imgu) +{ + imgu->imgu = openSubdevice(imgu->mediaDevice_, imgu->imguName_); + if (!imgu->imgu) + return -ENODEV; + + imgu->input = openDevice(imgu->mediaDevice_, + imgu->imguName_ + " input"); + if (!imgu->input) + goto error_delete_imgu; + + imgu->output = openDevice(imgu->mediaDevice_, + imgu->imguName_ + " output"); + if (!imgu->output) + goto error_delete_input; + + imgu->viewfinder = openDevice(imgu->mediaDevice_, + imgu->imguName_ + " viewfinder"); + if (!imgu->viewfinder) + goto error_delete_output; + + imgu->stat = openDevice(imgu->mediaDevice_, + imgu->imguName_ + " 3a stat"); + if (!imgu->stat) + goto error_delete_vf; + + return 0; + +error_delete_vf: + delete imgu->viewfinder; +error_delete_output: + delete imgu->output; +error_delete_input: + delete imgu->input; +error_delete_imgu: + delete imgu->imgu; + + return -ENODEV; +} + /** * \brief Initialize components of the CIO2 device \a index used by a camera * \param index The CIO2 device index @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2) if (ret) goto error_delete_csi2; - entity = cio2MediaDev_->getEntityByName(cio2Name); - if (!entity) { - LOG(IPU3, Error) - << "Failed to get entity '" << cio2Name << "'"; - ret = -EINVAL; + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); + if (!cio2->output) 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: @@ -483,6 +656,16 @@ error_delete_sensor: return ret; } +/** + * \brief Delete all devices associated with a CIO2 unit + */ +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) +{ + delete cio2->output; + delete cio2->csi2; + delete cio2->sensor; +} + /* * Cameras are created associating an image sensor (represented by a * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras() * image sensor is connected to it. */ unsigned int numCameras = 0; - for (unsigned int id = 0; id < 4; ++id) { + for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) { std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>(this); std::set<Stream *> streams{ &data->stream_ }; @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras() if (ret) continue; + /** + * \todo Dynamically assign ImgU devices; as of now, limit + * support to two cameras only, and assign imgu0 to the first + * one and imgu1 to the second. + */ + data->imgu = &imgus_[numCameras]; + ret = initImgU(data->imgu); + if (ret) { + deleteCIO2(cio2); + continue; + } + std::string cameraName = cio2->sensor->deviceName() + " " + std::to_string(id); std::shared_ptr<Camera> camera = Camera::create(this,
Create video devices and subdevices associated with an ImgU unit at camera registration time. Statically assign imgu0 to the first camera and imgu1 to the second one and limit support to two camera. This will have to be revised in future. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 223 +++++++++++++++++++++++++-- 1 file changed, 209 insertions(+), 14 deletions(-)