| Message ID | 20190228200410.3022-4-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:03PM +0100, Jacopo Mondi wrote: > Create video devices and subdevices associated with an ImgU unit, and > link the entities in the media graph to prepare the device for capture > operations at stream configuration time. > > As we support a single stream at the moment, always select imgu0. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++-- > 1 file changed, 207 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 4f1ab72debf8..9fa59c1bc97e 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -24,6 +24,28 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +struct ImguDevice { s/ImguDevice/ImgUDevice/ ? > + ImguDevice() > + : imgu(nullptr), input(nullptr), output(nullptr), > + viewfinder(nullptr), stat(nullptr) {} { } > + > + ~ImguDevice() > + { > + delete imgu; > + delete input; > + delete output; > + delete viewfinder; > + delete stat; > + } Unlike for the CIO2Device, this could be made a class, as I expect it will have more code associated with it, and the ImgU instances will be shared between the multiple camera instances. The linkImgu function would be a good candidate. > + > + V4L2Subdevice *imgu; > + V4L2Device *input; > + V4L2Device *output; > + V4L2Device *viewfinder; > + V4L2Device *stat; > + /* TODO: add param video device for 3A tuning */ \todo for consistency, even if this isn't a doxygen comment ? > +}; > + > struct Cio2Device { > Cio2Device() > : output(nullptr), csi2(nullptr), sensor(nullptr) {} > @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData > { > public: > Cio2Device cio2; > + ImguDevice *imgu; > > Stream stream_; > }; > @@ -71,6 +94,10 @@ public: > bool match(DeviceEnumerator *enumerator); > > private: > + static constexpr unsigned int IMGU_PAD_INPUT = 0; > + static constexpr unsigned int IMGU_PAD_OUTPUT = 2; > + static constexpr unsigned int IMGU_PAD_VF = 3; > + static constexpr unsigned int IMGU_PAD_STAT = 4; > static constexpr unsigned int IPU3_BUF_NUM = 4; I'd move that to the ImgUDevice struct/class. You can then remove the IMGU_ prefix. > > IPU3CameraData *cameraData(const Camera *camera) > @@ -79,9 +106,17 @@ private: > PipelineHandler::cameraData(camera)); > } > > + int linkImgu(ImguDevice *imgu); > + > + V4L2Device *openDevice(MediaDevice *media, std::string &name); > + V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name); > + int initImgu(ImguDevice *imgu); > int initCio2(unsigned int index, Cio2Device *cio2); > void registerCameras(); > > + ImguDevice imgu0_; > + ImguDevice imgu1_; Maybe an array with two entries ? > + > std::shared_ptr<MediaDevice> cio2MediaDev_; > std::shared_ptr<MediaDevice> imguMediaDev_; > }; > @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > V4L2DeviceFormat devFormat = {}; > int ret; > > + /* > + * TODO: dynamically assign ImgU devices; as of now, with a single > + * stream supported, always use 'imgu0'. /** * \todo ? > + */ > + data->imgu = &imgu0_; How about moving this to camera creation time, and assigned imgu0 to the first sensor and imgu1 to the second one ? > + ret = linkImgu(data->imgu); > + if (ret) > + return ret; > + > /* > * FIXME: as of now, the format gets applied to the sensor and is > * propagated along the pipeline. It should instead be applied on the > @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > if (cio2MediaDev_->open()) > goto error_release_mdev; > > + if (imguMediaDev_->open()) > + goto error_close_mdev; > + > if (cio2MediaDev_->disableLinks()) > - goto error_close_cio2; > + goto error_close_mdev; > + > + if (initImgu(&imgu0_)) > + goto error_close_mdev; > + > + if (initImgu(&imgu1_)) > + goto error_close_mdev; > + > > registerCameras(); > > cio2MediaDev_->close(); > + imguMediaDev_->close(); > > return true; > > -error_close_cio2: > +error_close_mdev: > cio2MediaDev_->close(); > + imguMediaDev_->close(); Error handling will be simplified when you'll rebase. I'd go as far as calling close() in the PipelineHandlerIPU3 destructor and remove the error path here. > > error_release_mdev: > cio2MediaDev_->release(); > @@ -353,6 +409,153 @@ error_release_mdev: > return false; > } > > +/* Link entities in the ImgU unit to prepare for capture operations. */ > +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > +{ > + MediaLink *link; > + int ret; > + > + unsigned int index = imguDevice == &imgu0_ ? 0 : 1; > + std::string imguName = "ipu3-imgu " + std::to_string(index); > + std::string inputName = imguName + " input"; > + std::string outputName = imguName + " output"; > + std::string viewfinderName = imguName + " viewfinder"; > + std::string statName = imguName + " 3a stat"; > + > + ret = imguMediaDev_->open(); > + if (ret) > + return ret; > + > + ret = imguMediaDev_->disableLinks(); This isn't a good idea, as you will interfere with the other ImgU. You should enable/disable links selectively based on your needs. > + if (ret) { > + imguMediaDev_->close(); > + return ret; > + } > + > + /* Link entities to configure the IMGU unit for capture. */ > + link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT); > + if (!link) { > + LOG(IPU3, Error) > + << "Failed to get link '" << inputName << "':0 -> '" > + << imguName << "':0"; Ideally you should use the IMGU_PAD_* constants instead of hardcoding them in error messages. > + ret = -ENODEV; > + goto error_close_mediadev; > + } > + link->setEnabled(true); > + > + link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0); > + if (!link) { > + LOG(IPU3, Error) > + << "Failed to get link '" << imguName << "':2 -> '" > + << outputName << "':0"; > + ret = -ENODEV; > + goto error_close_mediadev; > + } > + link->setEnabled(true); > + > + link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0); > + if (!link) { > + LOG(IPU3, Error) > + << "Failed to get link '" << imguName << "':3 -> '" > + << viewfinderName << "':0"; > + ret = -ENODEV; > + goto error_close_mediadev; > + } > + link->setEnabled(true); We have a single stream, why do you enable both output and viewfinder ? > + > + link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0); > + if (!link) { > + LOG(IPU3, Error) > + << "Failed to get link '" << imguName << "':4 -> '" > + << statName << "':0"; > + ret = -ENODEV; > + goto error_close_mediadev; > + } > + link->setEnabled(true); Same here, we don't make use of stats yes, there's no need to enable this link. > + > + imguMediaDev_->close(); > + > + return 0; > + > +error_close_mediadev: > + imguMediaDev_->close(); We really really need to think about how to handle open/close and write down a set of rules. Please make sure this is captured in a \todo. > + > + return ret; > + > +} > + > +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, > + std::string &name) const std::string & Same below. > +{ > + V4L2Device *dev; You can move this line down to declare and initialize the variable at the same time. > + > + MediaEntity *entity = media->getEntityByName(name); > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << name << "'"; > + return nullptr; > + } > + > + dev = new V4L2Device(entity); > + if (dev->open()) > + return nullptr; You'll leak dev, a delete is needed. > + > + return dev; > +} > + > +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, > + std::string &name) > +{ > + V4L2Subdevice *dev; > + > + MediaEntity *entity = media->getEntityByName(name); > + if (!entity) { > + LOG(IPU3, Error) > + << "Failed to get entity '" << name << "'"; > + return nullptr; > + } > + > + dev = new V4L2Subdevice(entity); > + if (dev->open()) Leak. > + return nullptr; > + > + return dev; > +} These two functions may be candidates for future generic helpers. Could you document them and add a \todo to mention this ? > + > +/* Create video devices and subdevices for the ImgU instance. */ > +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu) > +{ > + unsigned int index = imgu == &imgu0_ ? 0 : 1; A bit ugly, how about adding an index field to the ImguDevice structure ? > + std::string imguName = "ipu3-imgu " + std::to_string(index); > + std::string devName; > + > + imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName); > + if (!imgu->imgu) > + return -ENODEV; > + > + devName = imguName + " input"; > + imgu->input = openDevice(imguMediaDev_.get(), devName); > + if (!imgu->input) > + return -ENODEV; > + > + devName = imguName + " output"; > + imgu->output = openDevice(imguMediaDev_.get(), devName); > + if (!imgu->output) > + return -ENODEV; > + > + devName = imguName + " viewfinder"; > + imgu->viewfinder = openDevice(imguMediaDev_.get(), devName); > + if (!imgu->viewfinder) > + return -ENODEV; > + > + devName = imguName + " 3a stat"; > + imgu->stat = openDevice(imguMediaDev_.get(), devName); You could drop devName and call imgu->state = openDevice(imguMediaDev_.get(), imguName + " 3a stat"); Up to you. > + if (!imgu->stat) > + return -ENODEV; > + > + return 0; > +} > + > int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > { > int ret; > @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > 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) > + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > + if (!cio2->output) > return ret; > > return 0;
Hi Laurent, On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote: > > Create video devices and subdevices associated with an ImgU unit, and > > link the entities in the media graph to prepare the device for capture > > operations at stream configuration time. > > > > As we support a single stream at the moment, always select imgu0. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++-- > > 1 file changed, 207 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 4f1ab72debf8..9fa59c1bc97e 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -24,6 +24,28 @@ namespace libcamera { > > > > LOG_DEFINE_CATEGORY(IPU3) > > > > +struct ImguDevice { > > s/ImguDevice/ImgUDevice/ ? I would like ImguDevice best, but I'll use ImgUDevice for consistency with the kernel documentation. > > > + ImguDevice() > > + : imgu(nullptr), input(nullptr), output(nullptr), > > + viewfinder(nullptr), stat(nullptr) {} > > { > } > > > + > > + ~ImguDevice() > > + { > > + delete imgu; > > + delete input; > > + delete output; > > + delete viewfinder; > > + delete stat; > > + } > > Unlike for the CIO2Device, this could be made a class, as I expect it > will have more code associated with it, and the ImgU instances will be > shared between the multiple camera instances. The linkImgu function > would be a good candidate. > I'll move that to this class. I will has well add a "disableLinks()" methods as you suggested below. > > + > > + V4L2Subdevice *imgu; > > + V4L2Device *input; > > + V4L2Device *output; > > + V4L2Device *viewfinder; > > + V4L2Device *stat; > > + /* TODO: add param video device for 3A tuning */ > > \todo for consistency, even if this isn't a doxygen comment ? > Will change all of these. > > +}; > > + > > struct Cio2Device { > > Cio2Device() > > : output(nullptr), csi2(nullptr), sensor(nullptr) {} > > @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData > > { > > public: > > Cio2Device cio2; > > + ImguDevice *imgu; > > > > Stream stream_; > > }; > > @@ -71,6 +94,10 @@ public: > > bool match(DeviceEnumerator *enumerator); > > > > private: > > + static constexpr unsigned int IMGU_PAD_INPUT = 0; > > + static constexpr unsigned int IMGU_PAD_OUTPUT = 2; > > + static constexpr unsigned int IMGU_PAD_VF = 3; > > + static constexpr unsigned int IMGU_PAD_STAT = 4; > > static constexpr unsigned int IPU3_BUF_NUM = 4; > > I'd move that to the ImgUDevice struct/class. You can then remove the > IMGU_ prefix. > Agreed. > > > > IPU3CameraData *cameraData(const Camera *camera) > > @@ -79,9 +106,17 @@ private: > > PipelineHandler::cameraData(camera)); > > } > > > > + int linkImgu(ImguDevice *imgu); > > + > > + V4L2Device *openDevice(MediaDevice *media, std::string &name); > > + V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name); > > + int initImgu(ImguDevice *imgu); > > int initCio2(unsigned int index, Cio2Device *cio2); > > void registerCameras(); > > > > + ImguDevice imgu0_; > > + ImguDevice imgu1_; > > Maybe an array with two entries ? > They're two, and will stay two, but yes, I'll see how it looks. > > + > > std::shared_ptr<MediaDevice> cio2MediaDev_; > > std::shared_ptr<MediaDevice> imguMediaDev_; > > }; > > @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > V4L2DeviceFormat devFormat = {}; > > int ret; > > > > + /* > > + * TODO: dynamically assign ImgU devices; as of now, with a single > > + * stream supported, always use 'imgu0'. > > /** > * \todo > > ? > > > + */ > > + data->imgu = &imgu0_; > > How about moving this to camera creation time, and assigned imgu0 to the > first sensor and imgu1 to the second one ? > I've put it here, as I assumed ImgU association with Cameras will depend on the number of required streams. If you confirm my understanding I would keep it here instead of moving it back and forth. > > + ret = linkImgu(data->imgu); > > + if (ret) > > + return ret; > > + > > /* > > * FIXME: as of now, the format gets applied to the sensor and is > > * propagated along the pipeline. It should instead be applied on the > > @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > if (cio2MediaDev_->open()) > > goto error_release_mdev; > > > > + if (imguMediaDev_->open()) > > + goto error_close_mdev; > > + > > if (cio2MediaDev_->disableLinks()) > > - goto error_close_cio2; > > + goto error_close_mdev; > > + > > + if (initImgu(&imgu0_)) > > + goto error_close_mdev; > > + > > + if (initImgu(&imgu1_)) > > + goto error_close_mdev; > > + > > > > registerCameras(); > > > > cio2MediaDev_->close(); > > + imguMediaDev_->close(); > > > > return true; > > > > -error_close_cio2: > > +error_close_mdev: > > cio2MediaDev_->close(); > > + imguMediaDev_->close(); > > Error handling will be simplified when you'll rebase. I'd go as far as > calling close() in the PipelineHandlerIPU3 destructor and remove the > error path here. > It has been simplified by rebase, yes. I'll see how moving close() in destructor looks like (I think it's good) > > > > error_release_mdev: > > cio2MediaDev_->release(); > > @@ -353,6 +409,153 @@ error_release_mdev: > > return false; > > } > > > > +/* Link entities in the ImgU unit to prepare for capture operations. */ > > +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > > +{ > > + MediaLink *link; > > + int ret; > > + > > + unsigned int index = imguDevice == &imgu0_ ? 0 : 1; > > + std::string imguName = "ipu3-imgu " + std::to_string(index); > > + std::string inputName = imguName + " input"; > > + std::string outputName = imguName + " output"; > > + std::string viewfinderName = imguName + " viewfinder"; > > + std::string statName = imguName + " 3a stat"; > > + > > + ret = imguMediaDev_->open(); > > + if (ret) > > + return ret; > > + > > + ret = imguMediaDev_->disableLinks(); > > This isn't a good idea, as you will interfere with the other ImgU. You > should enable/disable links selectively based on your needs. > As I've said, I'll make a per-imgu instance link disabling method. I hope it is enough to disable all links in an imgu instance and we don't have to go link by link... > > + if (ret) { > > + imguMediaDev_->close(); > > + return ret; > > + } > > + > > + /* Link entities to configure the IMGU unit for capture. */ > > + link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT); > > + if (!link) { > > + LOG(IPU3, Error) > > + << "Failed to get link '" << inputName << "':0 -> '" > > + << imguName << "':0"; > > Ideally you should use the IMGU_PAD_* constants instead of hardcoding > them in error messages. > Ah well, it's just more lines for a single printout, but ok. > > + ret = -ENODEV; > > + goto error_close_mediadev; > > + } > > + link->setEnabled(true); > > + > > + link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0); > > + if (!link) { > > + LOG(IPU3, Error) > > + << "Failed to get link '" << imguName << "':2 -> '" > > + << outputName << "':0"; > > + ret = -ENODEV; > > + goto error_close_mediadev; > > + } > > + link->setEnabled(true); > > + > > + link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0); > > + if (!link) { > > + LOG(IPU3, Error) > > + << "Failed to get link '" << imguName << "':3 -> '" > > + << viewfinderName << "':0"; > > + ret = -ENODEV; > > + goto error_close_mediadev; > > + } > > + link->setEnabled(true); > > We have a single stream, why do you enable both output and viewfinder ? > From my testings, if I keep them disabled (== not linked) the system hangs even if I'm only capturing from the ouput video device. I will do some more testing, as setting up those nodes, queuing and dequeuing buffers there requires some not-nice code to keep track of the buffer indexes, as you've noticed. > > + > > + link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0); > > + if (!link) { > > + LOG(IPU3, Error) > > + << "Failed to get link '" << imguName << "':4 -> '" > > + << statName << "':0"; > > + ret = -ENODEV; > > + goto error_close_mediadev; > > + } > > + link->setEnabled(true); > > Same here, we don't make use of stats yes, there's no need to enable > this link. > > > + > > + imguMediaDev_->close(); > > + > > + return 0; > > + > > +error_close_mediadev: > > + imguMediaDev_->close(); > > We really really need to think about how to handle open/close and write > down a set of rules. Please make sure this is captured in a \todo. > > > + > > + return ret; > > + > > +} > > + > > +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, > > + std::string &name) > > const std::string & > > Same below. > > > +{ > > + V4L2Device *dev; > > You can move this line down to declare and initialize the variable at > the same time. > > > + > > + MediaEntity *entity = media->getEntityByName(name); > > + if (!entity) { > > + LOG(IPU3, Error) > > + << "Failed to get entity '" << name << "'"; > > + return nullptr; > > + } > > + > > + dev = new V4L2Device(entity); > > + if (dev->open()) > > + return nullptr; > > You'll leak dev, a delete is needed. > Ah, right! will fix. > > + > > + return dev; > > +} > > + > > +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, > > + std::string &name) > > +{ > > + V4L2Subdevice *dev; > > + > > + MediaEntity *entity = media->getEntityByName(name); > > + if (!entity) { > > + LOG(IPU3, Error) > > + << "Failed to get entity '" << name << "'"; > > + return nullptr; > > + } > > + > > + dev = new V4L2Subdevice(entity); > > + if (dev->open()) > > Leak. > > > + return nullptr; > > + > > + return dev; > > +} > > These two functions may be candidates for future generic helpers. Could > you document them and add a \todo to mention this ? > Agreed! > > + > > +/* Create video devices and subdevices for the ImgU instance. */ > > +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu) > > +{ > > + unsigned int index = imgu == &imgu0_ ? 0 : 1; > > A bit ugly, how about adding an index field to the ImguDevice structure > ? Yes. But when would you intialize such a field? at match time? In this function? I'll see how it'll look > > > + std::string imguName = "ipu3-imgu " + std::to_string(index); > > + std::string devName; > > + > > + imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName); > > + if (!imgu->imgu) > > + return -ENODEV; > > + > > + devName = imguName + " input"; > > + imgu->input = openDevice(imguMediaDev_.get(), devName); > > + if (!imgu->input) > > + return -ENODEV; > > + > > + devName = imguName + " output"; > > + imgu->output = openDevice(imguMediaDev_.get(), devName); > > + if (!imgu->output) > > + return -ENODEV; > > + > > + devName = imguName + " viewfinder"; > > + imgu->viewfinder = openDevice(imguMediaDev_.get(), devName); > > + if (!imgu->viewfinder) > > + return -ENODEV; > > + > > + devName = imguName + " 3a stat"; > > + imgu->stat = openDevice(imguMediaDev_.get(), devName); > > You could drop devName and call > > imgu->state = openDevice(imguMediaDev_.get(), imguName + " 3a stat"); > > Up to you. Less lines, so it's better Thanks j > > > + if (!imgu->stat) > > + return -ENODEV; > > + > > + return 0; > > +} > > + > > int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > > { > > int ret; > > @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > > 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) > > + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > > + if (!cio2->output) > > return ret; > > > > return 0; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Sat, Mar 09, 2019 at 09:50:18PM +0100, Jacopo Mondi wrote: > On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote: > > On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote: > >> Create video devices and subdevices associated with an ImgU unit, and > >> link the entities in the media graph to prepare the device for capture > >> operations at stream configuration time. > >> > >> As we support a single stream at the moment, always select imgu0. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++-- > >> 1 file changed, 207 insertions(+), 12 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 4f1ab72debf8..9fa59c1bc97e 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -24,6 +24,28 @@ namespace libcamera { > >> > >> LOG_DEFINE_CATEGORY(IPU3) > >> > >> +struct ImguDevice { > > > > s/ImguDevice/ImgUDevice/ ? > > I would like ImguDevice best, but I'll use ImgUDevice for consistency > with the kernel documentation. > > >> + ImguDevice() > >> + : imgu(nullptr), input(nullptr), output(nullptr), > >> + viewfinder(nullptr), stat(nullptr) {} > > > > { > > } > > > >> + > >> + ~ImguDevice() > >> + { > >> + delete imgu; > >> + delete input; > >> + delete output; > >> + delete viewfinder; > >> + delete stat; > >> + } > > > > Unlike for the CIO2Device, this could be made a class, as I expect it > > will have more code associated with it, and the ImgU instances will be > > shared between the multiple camera instances. The linkImgu function > > would be a good candidate. > > I'll move that to this class. I will has well add a "disableLinks()" > methods as you suggested below. > > >> + > >> + V4L2Subdevice *imgu; > >> + V4L2Device *input; > >> + V4L2Device *output; > >> + V4L2Device *viewfinder; > >> + V4L2Device *stat; > >> + /* TODO: add param video device for 3A tuning */ > > > > \todo for consistency, even if this isn't a doxygen comment ? > > Will change all of these. > > >> +}; > >> + > >> struct Cio2Device { > >> Cio2Device() > >> : output(nullptr), csi2(nullptr), sensor(nullptr) {} > >> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData > >> { > >> public: > >> Cio2Device cio2; > >> + ImguDevice *imgu; > >> > >> Stream stream_; > >> }; > >> @@ -71,6 +94,10 @@ public: > >> bool match(DeviceEnumerator *enumerator); > >> > >> private: > >> + static constexpr unsigned int IMGU_PAD_INPUT = 0; > >> + static constexpr unsigned int IMGU_PAD_OUTPUT = 2; > >> + static constexpr unsigned int IMGU_PAD_VF = 3; > >> + static constexpr unsigned int IMGU_PAD_STAT = 4; > >> static constexpr unsigned int IPU3_BUF_NUM = 4; > > > > I'd move that to the ImgUDevice struct/class. You can then remove the > > IMGU_ prefix. > > Agreed. > > >> > >> IPU3CameraData *cameraData(const Camera *camera) > >> @@ -79,9 +106,17 @@ private: > >> PipelineHandler::cameraData(camera)); > >> } > >> > >> + int linkImgu(ImguDevice *imgu); > >> + > >> + V4L2Device *openDevice(MediaDevice *media, std::string &name); > >> + V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name); > >> + int initImgu(ImguDevice *imgu); > >> int initCio2(unsigned int index, Cio2Device *cio2); > >> void registerCameras(); > >> > >> + ImguDevice imgu0_; > >> + ImguDevice imgu1_; > > > > Maybe an array with two entries ? > > They're two, and will stay two, but yes, I'll see how it looks. > > >> + > >> std::shared_ptr<MediaDevice> cio2MediaDev_; > >> std::shared_ptr<MediaDevice> imguMediaDev_; > >> }; > >> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > >> V4L2DeviceFormat devFormat = {}; > >> int ret; > >> > >> + /* > >> + * TODO: dynamically assign ImgU devices; as of now, with a single > >> + * stream supported, always use 'imgu0'. > > > > /** > > * \todo > > > > ? > > > >> + */ > >> + data->imgu = &imgu0_; > > > > How about moving this to camera creation time, and assigned imgu0 to the > > first sensor and imgu1 to the second one ? > > > > I've put it here, as I assumed ImgU association with Cameras will > depend on the number of required streams. If you confirm my > understanding I would keep it here instead of moving it back and > forth. Your understanding is correct, but with this implementation imgu0 is used for both cameras, so we can only use one camera at a time. If we assign imgu0 to the first camera and imgu1 to the second one we can start using both cameras, with up to two streams each, and later expand that to more streams. > >> + ret = linkImgu(data->imgu); > >> + if (ret) > >> + return ret; > >> + > >> /* > >> * FIXME: as of now, the format gets applied to the sensor and is > >> * propagated along the pipeline. It should instead be applied on the > >> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > >> if (cio2MediaDev_->open()) > >> goto error_release_mdev; > >> > >> + if (imguMediaDev_->open()) > >> + goto error_close_mdev; > >> + > >> if (cio2MediaDev_->disableLinks()) > >> - goto error_close_cio2; > >> + goto error_close_mdev; > >> + > >> + if (initImgu(&imgu0_)) > >> + goto error_close_mdev; > >> + > >> + if (initImgu(&imgu1_)) > >> + goto error_close_mdev; > >> + > >> > >> registerCameras(); > >> > >> cio2MediaDev_->close(); > >> + imguMediaDev_->close(); > >> > >> return true; > >> > >> -error_close_cio2: > >> +error_close_mdev: > >> cio2MediaDev_->close(); > >> + imguMediaDev_->close(); > > > > Error handling will be simplified when you'll rebase. I'd go as far as > > calling close() in the PipelineHandlerIPU3 destructor and remove the > > error path here. > > It has been simplified by rebase, yes. I'll see how moving close() in > destructor looks like (I think it's good) > > >> > >> error_release_mdev: > >> cio2MediaDev_->release(); > >> @@ -353,6 +409,153 @@ error_release_mdev: > >> return false; > >> } > >> > >> +/* Link entities in the ImgU unit to prepare for capture operations. */ > >> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > >> +{ > >> + MediaLink *link; > >> + int ret; > >> + > >> + unsigned int index = imguDevice == &imgu0_ ? 0 : 1; > >> + std::string imguName = "ipu3-imgu " + std::to_string(index); > >> + std::string inputName = imguName + " input"; > >> + std::string outputName = imguName + " output"; > >> + std::string viewfinderName = imguName + " viewfinder"; > >> + std::string statName = imguName + " 3a stat"; > >> + > >> + ret = imguMediaDev_->open(); > >> + if (ret) > >> + return ret; > >> + > >> + ret = imguMediaDev_->disableLinks(); > > > > This isn't a good idea, as you will interfere with the other ImgU. You > > should enable/disable links selectively based on your needs. > > As I've said, I'll make a per-imgu instance link disabling method. I > hope it is enough to disable all links in an imgu instance and we > don't have to go link by link... I haven't checked recently, are the two ImgUs exposed as separate media devices ? If so that's fine, otherwise you'll have to go link by link. > >> + if (ret) { > >> + imguMediaDev_->close(); > >> + return ret; > >> + } > >> + > >> + /* Link entities to configure the IMGU unit for capture. */ > >> + link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT); > >> + if (!link) { > >> + LOG(IPU3, Error) > >> + << "Failed to get link '" << inputName << "':0 -> '" > >> + << imguName << "':0"; > > > > Ideally you should use the IMGU_PAD_* constants instead of hardcoding > > them in error messages. > > Ah well, it's just more lines for a single printout, but ok. > > >> + ret = -ENODEV; > >> + goto error_close_mediadev; > >> + } > >> + link->setEnabled(true); > >> + > >> + link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0); > >> + if (!link) { > >> + LOG(IPU3, Error) > >> + << "Failed to get link '" << imguName << "':2 -> '" > >> + << outputName << "':0"; > >> + ret = -ENODEV; > >> + goto error_close_mediadev; > >> + } > >> + link->setEnabled(true); > >> + > >> + link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0); > >> + if (!link) { > >> + LOG(IPU3, Error) > >> + << "Failed to get link '" << imguName << "':3 -> '" > >> + << viewfinderName << "':0"; > >> + ret = -ENODEV; > >> + goto error_close_mediadev; > >> + } > >> + link->setEnabled(true); > > > > We have a single stream, why do you enable both output and viewfinder ? > > From my testings, if I keep them disabled (== not linked) the system > hangs even if I'm only capturing from the ouput video device. I will > do some more testing, as setting up those nodes, queuing and dequeuing > buffers there requires some not-nice code to keep track of the buffer > indexes, as you've noticed. Another kernel bug fix candidate :-) > >> + > >> + link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0); > >> + if (!link) { > >> + LOG(IPU3, Error) > >> + << "Failed to get link '" << imguName << "':4 -> '" > >> + << statName << "':0"; > >> + ret = -ENODEV; > >> + goto error_close_mediadev; > >> + } > >> + link->setEnabled(true); > > > > Same here, we don't make use of stats yes, there's no need to enable > > this link. > > > >> + > >> + imguMediaDev_->close(); > >> + > >> + return 0; > >> + > >> +error_close_mediadev: > >> + imguMediaDev_->close(); > > > > We really really need to think about how to handle open/close and write > > down a set of rules. Please make sure this is captured in a \todo. > > > >> + > >> + return ret; > >> + > >> +} > >> + > >> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, > >> + std::string &name) > > > > const std::string & > > > > Same below. > > > >> +{ > >> + V4L2Device *dev; > > > > You can move this line down to declare and initialize the variable at > > the same time. > > > >> + > >> + MediaEntity *entity = media->getEntityByName(name); > >> + if (!entity) { > >> + LOG(IPU3, Error) > >> + << "Failed to get entity '" << name << "'"; > >> + return nullptr; > >> + } > >> + > >> + dev = new V4L2Device(entity); > >> + if (dev->open()) > >> + return nullptr; > > > > You'll leak dev, a delete is needed. > > Ah, right! will fix. > > >> + > >> + return dev; > >> +} > >> + > >> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, > >> + std::string &name) > >> +{ > >> + V4L2Subdevice *dev; > >> + > >> + MediaEntity *entity = media->getEntityByName(name); > >> + if (!entity) { > >> + LOG(IPU3, Error) > >> + << "Failed to get entity '" << name << "'"; > >> + return nullptr; > >> + } > >> + > >> + dev = new V4L2Subdevice(entity); > >> + if (dev->open()) > > > > Leak. > > > >> + return nullptr; > >> + > >> + return dev; > >> +} > > > > These two functions may be candidates for future generic helpers. Could > > you document them and add a \todo to mention this ? > > Agreed! > > >> + > >> +/* Create video devices and subdevices for the ImgU instance. */ > >> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu) > >> +{ > >> + unsigned int index = imgu == &imgu0_ ? 0 : 1; > > > > A bit ugly, how about adding an index field to the ImguDevice structure > > ? > > Yes. But when would you intialize such a field? at match time? In this > function? I'll see how it'll look I think I'd do it at match time. > >> + std::string imguName = "ipu3-imgu " + std::to_string(index); > >> + std::string devName; > >> + > >> + imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName); > >> + if (!imgu->imgu) > >> + return -ENODEV; > >> + > >> + devName = imguName + " input"; > >> + imgu->input = openDevice(imguMediaDev_.get(), devName); > >> + if (!imgu->input) > >> + return -ENODEV; > >> + > >> + devName = imguName + " output"; > >> + imgu->output = openDevice(imguMediaDev_.get(), devName); > >> + if (!imgu->output) > >> + return -ENODEV; > >> + > >> + devName = imguName + " viewfinder"; > >> + imgu->viewfinder = openDevice(imguMediaDev_.get(), devName); > >> + if (!imgu->viewfinder) > >> + return -ENODEV; > >> + > >> + devName = imguName + " 3a stat"; > >> + imgu->stat = openDevice(imguMediaDev_.get(), devName); > > > > You could drop devName and call > > > > imgu->state = openDevice(imguMediaDev_.get(), imguName + " 3a stat"); > > > > Up to you. > > Less lines, so it's better > > >> + if (!imgu->stat) > >> + return -ENODEV; > >> + > >> + return 0; > >> +} > >> + > >> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > >> { > >> int ret; > >> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > >> 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) > >> + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > >> + if (!cio2->output) > >> return ret; > >> > >> return 0;
Hi Laurent, On Sun, Mar 10, 2019 at 03:08:33PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Sat, Mar 09, 2019 at 09:50:18PM +0100, Jacopo Mondi wrote: > > On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote: > > > On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote: > > >> Create video devices and subdevices associated with an ImgU unit, and > > >> link the entities in the media graph to prepare the device for capture > > >> operations at stream configuration time. > > >> > > >> As we support a single stream at the moment, always select imgu0. > > >> > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >> --- > > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++-- > > >> 1 file changed, 207 insertions(+), 12 deletions(-) > > >> > > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> index 4f1ab72debf8..9fa59c1bc97e 100644 > > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> @@ -24,6 +24,28 @@ namespace libcamera { > > >> > > >> LOG_DEFINE_CATEGORY(IPU3) > > >> > > >> +struct ImguDevice { > > > > > > s/ImguDevice/ImgUDevice/ ? > > > > I would like ImguDevice best, but I'll use ImgUDevice for consistency > > with the kernel documentation. > > > > >> + ImguDevice() > > >> + : imgu(nullptr), input(nullptr), output(nullptr), > > >> + viewfinder(nullptr), stat(nullptr) {} > > > > > > { > > > } > > > > > >> + > > >> + ~ImguDevice() > > >> + { > > >> + delete imgu; > > >> + delete input; > > >> + delete output; > > >> + delete viewfinder; > > >> + delete stat; > > >> + } > > > > > > Unlike for the CIO2Device, this could be made a class, as I expect it > > > will have more code associated with it, and the ImgU instances will be > > > shared between the multiple camera instances. The linkImgu function > > > would be a good candidate. > > > > I'll move that to this class. I will has well add a "disableLinks()" > > methods as you suggested below. > > > > >> + > > >> + V4L2Subdevice *imgu; > > >> + V4L2Device *input; > > >> + V4L2Device *output; > > >> + V4L2Device *viewfinder; > > >> + V4L2Device *stat; > > >> + /* TODO: add param video device for 3A tuning */ > > > > > > \todo for consistency, even if this isn't a doxygen comment ? > > > > Will change all of these. > > > > >> +}; > > >> + > > >> struct Cio2Device { > > >> Cio2Device() > > >> : output(nullptr), csi2(nullptr), sensor(nullptr) {} > > >> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData > > >> { > > >> public: > > >> Cio2Device cio2; > > >> + ImguDevice *imgu; > > >> > > >> Stream stream_; > > >> }; > > >> @@ -71,6 +94,10 @@ public: > > >> bool match(DeviceEnumerator *enumerator); > > >> > > >> private: > > >> + static constexpr unsigned int IMGU_PAD_INPUT = 0; > > >> + static constexpr unsigned int IMGU_PAD_OUTPUT = 2; > > >> + static constexpr unsigned int IMGU_PAD_VF = 3; > > >> + static constexpr unsigned int IMGU_PAD_STAT = 4; > > >> static constexpr unsigned int IPU3_BUF_NUM = 4; > > > > > > I'd move that to the ImgUDevice struct/class. You can then remove the > > > IMGU_ prefix. > > > > Agreed. > > > > >> > > >> IPU3CameraData *cameraData(const Camera *camera) > > >> @@ -79,9 +106,17 @@ private: > > >> PipelineHandler::cameraData(camera)); > > >> } > > >> > > >> + int linkImgu(ImguDevice *imgu); > > >> + > > >> + V4L2Device *openDevice(MediaDevice *media, std::string &name); > > >> + V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name); > > >> + int initImgu(ImguDevice *imgu); > > >> int initCio2(unsigned int index, Cio2Device *cio2); > > >> void registerCameras(); > > >> > > >> + ImguDevice imgu0_; > > >> + ImguDevice imgu1_; > > > > > > Maybe an array with two entries ? > > > > They're two, and will stay two, but yes, I'll see how it looks. > > > > >> + > > >> std::shared_ptr<MediaDevice> cio2MediaDev_; > > >> std::shared_ptr<MediaDevice> imguMediaDev_; > > >> }; > > >> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > >> V4L2DeviceFormat devFormat = {}; > > >> int ret; > > >> > > >> + /* > > >> + * TODO: dynamically assign ImgU devices; as of now, with a single > > >> + * stream supported, always use 'imgu0'. > > > > > > /** > > > * \todo > > > > > > ? > > > > > >> + */ > > >> + data->imgu = &imgu0_; > > > > > > How about moving this to camera creation time, and assigned imgu0 to the > > > first sensor and imgu1 to the second one ? > > > > > > > I've put it here, as I assumed ImgU association with Cameras will > > depend on the number of required streams. If you confirm my > > understanding I would keep it here instead of moving it back and > > forth. > > Your understanding is correct, but with this implementation imgu0 is > used for both cameras, so we can only use one camera at a time. If we > assign imgu0 to the first camera and imgu1 to the second one we can > start using both cameras, with up to two streams each, and later expand > that to more streams. > I see. I'll do the assignement there (and limit the number of supported cameras to two for now) > > >> + ret = linkImgu(data->imgu); > > >> + if (ret) > > >> + return ret; > > >> + > > >> /* > > >> * FIXME: as of now, the format gets applied to the sensor and is > > >> * propagated along the pipeline. It should instead be applied on the > > >> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > >> if (cio2MediaDev_->open()) > > >> goto error_release_mdev; > > >> > > >> + if (imguMediaDev_->open()) > > >> + goto error_close_mdev; > > >> + > > >> if (cio2MediaDev_->disableLinks()) > > >> - goto error_close_cio2; > > >> + goto error_close_mdev; > > >> + > > >> + if (initImgu(&imgu0_)) > > >> + goto error_close_mdev; > > >> + > > >> + if (initImgu(&imgu1_)) > > >> + goto error_close_mdev; > > >> + > > >> > > >> registerCameras(); > > >> > > >> cio2MediaDev_->close(); > > >> + imguMediaDev_->close(); > > >> > > >> return true; > > >> > > >> -error_close_cio2: > > >> +error_close_mdev: > > >> cio2MediaDev_->close(); > > >> + imguMediaDev_->close(); > > > > > > Error handling will be simplified when you'll rebase. I'd go as far as > > > calling close() in the PipelineHandlerIPU3 destructor and remove the > > > error path here. > > > > It has been simplified by rebase, yes. I'll see how moving close() in > > destructor looks like (I think it's good) > > Actually, the pipeline handler will be destroyed at library tear-down time, isn't it? So I don't think we can rely on its destructor to close the media devices... > > >> > > >> error_release_mdev: > > >> cio2MediaDev_->release(); > > >> @@ -353,6 +409,153 @@ error_release_mdev: > > >> return false; > > >> } > > >> > > >> +/* Link entities in the ImgU unit to prepare for capture operations. */ > > >> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > > >> +{ > > >> + MediaLink *link; > > >> + int ret; > > >> + > > >> + unsigned int index = imguDevice == &imgu0_ ? 0 : 1; > > >> + std::string imguName = "ipu3-imgu " + std::to_string(index); > > >> + std::string inputName = imguName + " input"; > > >> + std::string outputName = imguName + " output"; > > >> + std::string viewfinderName = imguName + " viewfinder"; > > >> + std::string statName = imguName + " 3a stat"; > > >> + > > >> + ret = imguMediaDev_->open(); > > >> + if (ret) > > >> + return ret; > > >> + > > >> + ret = imguMediaDev_->disableLinks(); > > > > > > This isn't a good idea, as you will interfere with the other ImgU. You > > > should enable/disable links selectively based on your needs. > > > > As I've said, I'll make a per-imgu instance link disabling method. I > > hope it is enough to disable all links in an imgu instance and we > > don't have to go link by link... > > I haven't checked recently, are the two ImgUs exposed as separate media > devices ? If so that's fine, otherwise you'll have to go link by link. > No, the imgu units are part of the same media device. But it's fine anyway, I will create a method that goes link by link :( > > >> + if (ret) { > > >> + imguMediaDev_->close(); > > >> + return ret; > > >> + } > > >> + > > >> + /* Link entities to configure the IMGU unit for capture. */ > > >> + link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT); > > >> + if (!link) { > > >> + LOG(IPU3, Error) > > >> + << "Failed to get link '" << inputName << "':0 -> '" > > >> + << imguName << "':0"; > > > > > > Ideally you should use the IMGU_PAD_* constants instead of hardcoding > > > them in error messages. > > > > Ah well, it's just more lines for a single printout, but ok. > > > > >> + ret = -ENODEV; > > >> + goto error_close_mediadev; > > >> + } > > >> + link->setEnabled(true); > > >> + > > >> + link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0); > > >> + if (!link) { > > >> + LOG(IPU3, Error) > > >> + << "Failed to get link '" << imguName << "':2 -> '" > > >> + << outputName << "':0"; > > >> + ret = -ENODEV; > > >> + goto error_close_mediadev; > > >> + } > > >> + link->setEnabled(true); > > >> + > > >> + link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0); > > >> + if (!link) { > > >> + LOG(IPU3, Error) > > >> + << "Failed to get link '" << imguName << "':3 -> '" > > >> + << viewfinderName << "':0"; > > >> + ret = -ENODEV; > > >> + goto error_close_mediadev; > > >> + } > > >> + link->setEnabled(true); > > > > > > We have a single stream, why do you enable both output and viewfinder ? > > > > From my testings, if I keep them disabled (== not linked) the system > > hangs even if I'm only capturing from the ouput video device. I will > > do some more testing, as setting up those nodes, queuing and dequeuing > > buffers there requires some not-nice code to keep track of the buffer > > indexes, as you've noticed. > > Another kernel bug fix candidate :-) > I plan to do more testing once I have included all these comments in v2. Will report how they go.. Thanks j > > >> + > > >> + link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0); > > >> + if (!link) { > > >> + LOG(IPU3, Error) > > >> + << "Failed to get link '" << imguName << "':4 -> '" > > >> + << statName << "':0"; > > >> + ret = -ENODEV; > > >> + goto error_close_mediadev; > > >> + } > > >> + link->setEnabled(true); > > > > > > Same here, we don't make use of stats yes, there's no need to enable > > > this link. > > > > > >> + > > >> + imguMediaDev_->close(); > > >> + > > >> + return 0; > > >> + > > >> +error_close_mediadev: > > >> + imguMediaDev_->close(); > > > > > > We really really need to think about how to handle open/close and write > > > down a set of rules. Please make sure this is captured in a \todo. > > > > > >> + > > >> + return ret; > > >> + > > >> +} > > >> + > > >> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, > > >> + std::string &name) > > > > > > const std::string & > > > > > > Same below. > > > > > >> +{ > > >> + V4L2Device *dev; > > > > > > You can move this line down to declare and initialize the variable at > > > the same time. > > > > > >> + > > >> + MediaEntity *entity = media->getEntityByName(name); > > >> + if (!entity) { > > >> + LOG(IPU3, Error) > > >> + << "Failed to get entity '" << name << "'"; > > >> + return nullptr; > > >> + } > > >> + > > >> + dev = new V4L2Device(entity); > > >> + if (dev->open()) > > >> + return nullptr; > > > > > > You'll leak dev, a delete is needed. > > > > Ah, right! will fix. > > > > >> + > > >> + return dev; > > >> +} > > >> + > > >> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, > > >> + std::string &name) > > >> +{ > > >> + V4L2Subdevice *dev; > > >> + > > >> + MediaEntity *entity = media->getEntityByName(name); > > >> + if (!entity) { > > >> + LOG(IPU3, Error) > > >> + << "Failed to get entity '" << name << "'"; > > >> + return nullptr; > > >> + } > > >> + > > >> + dev = new V4L2Subdevice(entity); > > >> + if (dev->open()) > > > > > > Leak. > > > > > >> + return nullptr; > > >> + > > >> + return dev; > > >> +} > > > > > > These two functions may be candidates for future generic helpers. Could > > > you document them and add a \todo to mention this ? > > > > Agreed! > > > > >> + > > >> +/* Create video devices and subdevices for the ImgU instance. */ > > >> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu) > > >> +{ > > >> + unsigned int index = imgu == &imgu0_ ? 0 : 1; > > > > > > A bit ugly, how about adding an index field to the ImguDevice structure > > > ? > > > > Yes. But when would you intialize such a field? at match time? In this > > function? I'll see how it'll look > > I think I'd do it at match time. > > > >> + std::string imguName = "ipu3-imgu " + std::to_string(index); > > >> + std::string devName; > > >> + > > >> + imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName); > > >> + if (!imgu->imgu) > > >> + return -ENODEV; > > >> + > > >> + devName = imguName + " input"; > > >> + imgu->input = openDevice(imguMediaDev_.get(), devName); > > >> + if (!imgu->input) > > >> + return -ENODEV; > > >> + > > >> + devName = imguName + " output"; > > >> + imgu->output = openDevice(imguMediaDev_.get(), devName); > > >> + if (!imgu->output) > > >> + return -ENODEV; > > >> + > > >> + devName = imguName + " viewfinder"; > > >> + imgu->viewfinder = openDevice(imguMediaDev_.get(), devName); > > >> + if (!imgu->viewfinder) > > >> + return -ENODEV; > > >> + > > >> + devName = imguName + " 3a stat"; > > >> + imgu->stat = openDevice(imguMediaDev_.get(), devName); > > > > > > You could drop devName and call > > > > > > imgu->state = openDevice(imguMediaDev_.get(), imguName + " 3a stat"); > > > > > > Up to you. > > > > Less lines, so it's better > > > > >> + if (!imgu->stat) > > >> + return -ENODEV; > > >> + > > >> + return 0; > > >> +} > > >> + > > >> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > > >> { > > >> int ret; > > >> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > > >> 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) > > >> + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > > >> + if (!cio2->output) > > >> return ret; > > >> > > >> return 0; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Mar 11, 2019 at 09:10:41AM +0100, Jacopo Mondi wrote: > On Sun, Mar 10, 2019 at 03:08:33PM +0200, Laurent Pinchart wrote: > > On Sat, Mar 09, 2019 at 09:50:18PM +0100, Jacopo Mondi wrote: > >> On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote: > >>> On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote: > >>>> Create video devices and subdevices associated with an ImgU unit, and > >>>> link the entities in the media graph to prepare the device for capture > >>>> operations at stream configuration time. > >>>> > >>>> As we support a single stream at the moment, always select imgu0. > >>>> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> --- > >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++-- > >>>> 1 file changed, 207 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> index 4f1ab72debf8..9fa59c1bc97e 100644 > >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> @@ -24,6 +24,28 @@ namespace libcamera { > >>>> > >>>> LOG_DEFINE_CATEGORY(IPU3) > >>>> > >>>> +struct ImguDevice { > >>> > >>> s/ImguDevice/ImgUDevice/ ? > >> > >> I would like ImguDevice best, but I'll use ImgUDevice for consistency > >> with the kernel documentation. > >> > >>>> + ImguDevice() > >>>> + : imgu(nullptr), input(nullptr), output(nullptr), > >>>> + viewfinder(nullptr), stat(nullptr) {} > >>> > >>> { > >>> } > >>> > >>>> + > >>>> + ~ImguDevice() > >>>> + { > >>>> + delete imgu; > >>>> + delete input; > >>>> + delete output; > >>>> + delete viewfinder; > >>>> + delete stat; > >>>> + } > >>> > >>> Unlike for the CIO2Device, this could be made a class, as I expect it > >>> will have more code associated with it, and the ImgU instances will be > >>> shared between the multiple camera instances. The linkImgu function > >>> would be a good candidate. > >> > >> I'll move that to this class. I will has well add a "disableLinks()" > >> methods as you suggested below. > >> > >>>> + > >>>> + V4L2Subdevice *imgu; > >>>> + V4L2Device *input; > >>>> + V4L2Device *output; > >>>> + V4L2Device *viewfinder; > >>>> + V4L2Device *stat; > >>>> + /* TODO: add param video device for 3A tuning */ > >>> > >>> \todo for consistency, even if this isn't a doxygen comment ? > >> > >> Will change all of these. > >> > >>>> +}; > >>>> + > >>>> struct Cio2Device { > >>>> Cio2Device() > >>>> : output(nullptr), csi2(nullptr), sensor(nullptr) {} > >>>> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData > >>>> { > >>>> public: > >>>> Cio2Device cio2; > >>>> + ImguDevice *imgu; > >>>> > >>>> Stream stream_; > >>>> }; > >>>> @@ -71,6 +94,10 @@ public: > >>>> bool match(DeviceEnumerator *enumerator); > >>>> > >>>> private: > >>>> + static constexpr unsigned int IMGU_PAD_INPUT = 0; > >>>> + static constexpr unsigned int IMGU_PAD_OUTPUT = 2; > >>>> + static constexpr unsigned int IMGU_PAD_VF = 3; > >>>> + static constexpr unsigned int IMGU_PAD_STAT = 4; > >>>> static constexpr unsigned int IPU3_BUF_NUM = 4; > >>> > >>> I'd move that to the ImgUDevice struct/class. You can then remove the > >>> IMGU_ prefix. > >> > >> Agreed. > >> > >>>> > >>>> IPU3CameraData *cameraData(const Camera *camera) > >>>> @@ -79,9 +106,17 @@ private: > >>>> PipelineHandler::cameraData(camera)); > >>>> } > >>>> > >>>> + int linkImgu(ImguDevice *imgu); > >>>> + > >>>> + V4L2Device *openDevice(MediaDevice *media, std::string &name); > >>>> + V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name); > >>>> + int initImgu(ImguDevice *imgu); > >>>> int initCio2(unsigned int index, Cio2Device *cio2); > >>>> void registerCameras(); > >>>> > >>>> + ImguDevice imgu0_; > >>>> + ImguDevice imgu1_; > >>> > >>> Maybe an array with two entries ? > >> > >> They're two, and will stay two, but yes, I'll see how it looks. > >> > >>>> + > >>>> std::shared_ptr<MediaDevice> cio2MediaDev_; > >>>> std::shared_ptr<MediaDevice> imguMediaDev_; > >>>> }; > >>>> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > >>>> V4L2DeviceFormat devFormat = {}; > >>>> int ret; > >>>> > >>>> + /* > >>>> + * TODO: dynamically assign ImgU devices; as of now, with a single > >>>> + * stream supported, always use 'imgu0'. > >>> > >>> /** > >>> * \todo > >>> > >>> ? > >>> > >>>> + */ > >>>> + data->imgu = &imgu0_; > >>> > >>> How about moving this to camera creation time, and assigned imgu0 to the > >>> first sensor and imgu1 to the second one ? > >>> > >> > >> I've put it here, as I assumed ImgU association with Cameras will > >> depend on the number of required streams. If you confirm my > >> understanding I would keep it here instead of moving it back and > >> forth. > > > > Your understanding is correct, but with this implementation imgu0 is > > used for both cameras, so we can only use one camera at a time. If we > > assign imgu0 to the first camera and imgu1 to the second one we can > > start using both cameras, with up to two streams each, and later expand > > that to more streams. > > > > I see. I'll do the assignement there (and limit the number of > supported cameras to two for now) > > >>>> + ret = linkImgu(data->imgu); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> /* > >>>> * FIXME: as of now, the format gets applied to the sensor and is > >>>> * propagated along the pipeline. It should instead be applied on the > >>>> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > >>>> if (cio2MediaDev_->open()) > >>>> goto error_release_mdev; > >>>> > >>>> + if (imguMediaDev_->open()) > >>>> + goto error_close_mdev; > >>>> + > >>>> if (cio2MediaDev_->disableLinks()) > >>>> - goto error_close_cio2; > >>>> + goto error_close_mdev; > >>>> + > >>>> + if (initImgu(&imgu0_)) > >>>> + goto error_close_mdev; > >>>> + > >>>> + if (initImgu(&imgu1_)) > >>>> + goto error_close_mdev; > >>>> + > >>>> > >>>> registerCameras(); > >>>> > >>>> cio2MediaDev_->close(); > >>>> + imguMediaDev_->close(); > >>>> > >>>> return true; > >>>> > >>>> -error_close_cio2: > >>>> +error_close_mdev: > >>>> cio2MediaDev_->close(); > >>>> + imguMediaDev_->close(); > >>> > >>> Error handling will be simplified when you'll rebase. I'd go as far as > >>> calling close() in the PipelineHandlerIPU3 destructor and remove the > >>> error path here. > >> > >> It has been simplified by rebase, yes. I'll see how moving close() in > >> destructor looks like (I think it's good) > >> > > Actually, the pipeline handler will be destroyed at library tear-down > time, isn't it? So I don't think we can rely on its destructor to close the > media devices... It will be destroyed at the earlier of CameraManager::stop() time, when the device is unplugged, or immediately after the match() function returns an error. I think we'll need to forward the camera open() and close() calls to the pipeline manager for normal operation. The error path could still rely on the destructor, but that would be assymetrical, so maybe not the best idea. Feel free to keep the calls here. > >>>> > >>>> error_release_mdev: > >>>> cio2MediaDev_->release(); > >>>> @@ -353,6 +409,153 @@ error_release_mdev: > >>>> return false; > >>>> } > >>>> > >>>> +/* Link entities in the ImgU unit to prepare for capture operations. */ > >>>> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > >>>> +{ > >>>> + MediaLink *link; > >>>> + int ret; > >>>> + > >>>> + unsigned int index = imguDevice == &imgu0_ ? 0 : 1; > >>>> + std::string imguName = "ipu3-imgu " + std::to_string(index); > >>>> + std::string inputName = imguName + " input"; > >>>> + std::string outputName = imguName + " output"; > >>>> + std::string viewfinderName = imguName + " viewfinder"; > >>>> + std::string statName = imguName + " 3a stat"; > >>>> + > >>>> + ret = imguMediaDev_->open(); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + ret = imguMediaDev_->disableLinks(); > >>> > >>> This isn't a good idea, as you will interfere with the other ImgU. You > >>> should enable/disable links selectively based on your needs. > >> > >> As I've said, I'll make a per-imgu instance link disabling method. I > >> hope it is enough to disable all links in an imgu instance and we > >> don't have to go link by link... > > > > I haven't checked recently, are the two ImgUs exposed as separate media > > devices ? If so that's fine, otherwise you'll have to go link by link. > > No, the imgu units are part of the same media device. But it's fine > anyway, I will create a method that goes link by link :( Just thinking out loud, would a helper in MediaDevice that would take a vector of links be useful for other pipeline handlers ? > >>>> + if (ret) { > >>>> + imguMediaDev_->close(); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + /* Link entities to configure the IMGU unit for capture. */ > >>>> + link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT); > >>>> + if (!link) { > >>>> + LOG(IPU3, Error) > >>>> + << "Failed to get link '" << inputName << "':0 -> '" > >>>> + << imguName << "':0"; > >>> > >>> Ideally you should use the IMGU_PAD_* constants instead of hardcoding > >>> them in error messages. > >> > >> Ah well, it's just more lines for a single printout, but ok. > >> > >>>> + ret = -ENODEV; > >>>> + goto error_close_mediadev; > >>>> + } > >>>> + link->setEnabled(true); > >>>> + > >>>> + link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0); > >>>> + if (!link) { > >>>> + LOG(IPU3, Error) > >>>> + << "Failed to get link '" << imguName << "':2 -> '" > >>>> + << outputName << "':0"; > >>>> + ret = -ENODEV; > >>>> + goto error_close_mediadev; > >>>> + } > >>>> + link->setEnabled(true); > >>>> + > >>>> + link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0); > >>>> + if (!link) { > >>>> + LOG(IPU3, Error) > >>>> + << "Failed to get link '" << imguName << "':3 -> '" > >>>> + << viewfinderName << "':0"; > >>>> + ret = -ENODEV; > >>>> + goto error_close_mediadev; > >>>> + } > >>>> + link->setEnabled(true); > >>> > >>> We have a single stream, why do you enable both output and viewfinder ? > >> > >> From my testings, if I keep them disabled (== not linked) the system > >> hangs even if I'm only capturing from the ouput video device. I will > >> do some more testing, as setting up those nodes, queuing and dequeuing > >> buffers there requires some not-nice code to keep track of the buffer > >> indexes, as you've noticed. > > > > Another kernel bug fix candidate :-) > > I plan to do more testing once I have included all these comments in > v2. Will report how they go.. > > >>>> + > >>>> + link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0); > >>>> + if (!link) { > >>>> + LOG(IPU3, Error) > >>>> + << "Failed to get link '" << imguName << "':4 -> '" > >>>> + << statName << "':0"; > >>>> + ret = -ENODEV; > >>>> + goto error_close_mediadev; > >>>> + } > >>>> + link->setEnabled(true); > >>> > >>> Same here, we don't make use of stats yes, there's no need to enable > >>> this link. > >>> > >>>> + > >>>> + imguMediaDev_->close(); > >>>> + > >>>> + return 0; > >>>> + > >>>> +error_close_mediadev: > >>>> + imguMediaDev_->close(); > >>> > >>> We really really need to think about how to handle open/close and write > >>> down a set of rules. Please make sure this is captured in a \todo. > >>> > >>>> + > >>>> + return ret; > >>>> + > >>>> +} > >>>> + > >>>> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, > >>>> + std::string &name) > >>> > >>> const std::string & > >>> > >>> Same below. > >>> > >>>> +{ > >>>> + V4L2Device *dev; > >>> > >>> You can move this line down to declare and initialize the variable at > >>> the same time. > >>> > >>>> + > >>>> + MediaEntity *entity = media->getEntityByName(name); > >>>> + if (!entity) { > >>>> + LOG(IPU3, Error) > >>>> + << "Failed to get entity '" << name << "'"; > >>>> + return nullptr; > >>>> + } > >>>> + > >>>> + dev = new V4L2Device(entity); > >>>> + if (dev->open()) > >>>> + return nullptr; > >>> > >>> You'll leak dev, a delete is needed. > >> > >> Ah, right! will fix. > >> > >>>> + > >>>> + return dev; > >>>> +} > >>>> + > >>>> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, > >>>> + std::string &name) > >>>> +{ > >>>> + V4L2Subdevice *dev; > >>>> + > >>>> + MediaEntity *entity = media->getEntityByName(name); > >>>> + if (!entity) { > >>>> + LOG(IPU3, Error) > >>>> + << "Failed to get entity '" << name << "'"; > >>>> + return nullptr; > >>>> + } > >>>> + > >>>> + dev = new V4L2Subdevice(entity); > >>>> + if (dev->open()) > >>> > >>> Leak. > >>> > >>>> + return nullptr; > >>>> + > >>>> + return dev; > >>>> +} > >>> > >>> These two functions may be candidates for future generic helpers. Could > >>> you document them and add a \todo to mention this ? > >> > >> Agreed! > >> > >>>> + > >>>> +/* Create video devices and subdevices for the ImgU instance. */ > >>>> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu) > >>>> +{ > >>>> + unsigned int index = imgu == &imgu0_ ? 0 : 1; > >>> > >>> A bit ugly, how about adding an index field to the ImguDevice structure > >>> ? > >> > >> Yes. But when would you intialize such a field? at match time? In this > >> function? I'll see how it'll look > > > > I think I'd do it at match time. > > > >>>> + std::string imguName = "ipu3-imgu " + std::to_string(index); > >>>> + std::string devName; > >>>> + > >>>> + imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName); > >>>> + if (!imgu->imgu) > >>>> + return -ENODEV; > >>>> + > >>>> + devName = imguName + " input"; > >>>> + imgu->input = openDevice(imguMediaDev_.get(), devName); > >>>> + if (!imgu->input) > >>>> + return -ENODEV; > >>>> + > >>>> + devName = imguName + " output"; > >>>> + imgu->output = openDevice(imguMediaDev_.get(), devName); > >>>> + if (!imgu->output) > >>>> + return -ENODEV; > >>>> + > >>>> + devName = imguName + " viewfinder"; > >>>> + imgu->viewfinder = openDevice(imguMediaDev_.get(), devName); > >>>> + if (!imgu->viewfinder) > >>>> + return -ENODEV; > >>>> + > >>>> + devName = imguName + " 3a stat"; > >>>> + imgu->stat = openDevice(imguMediaDev_.get(), devName); > >>> > >>> You could drop devName and call > >>> > >>> imgu->state = openDevice(imguMediaDev_.get(), imguName + " 3a stat"); > >>> > >>> Up to you. > >> > >> Less lines, so it's better > >> > >>>> + if (!imgu->stat) > >>>> + return -ENODEV; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > >>>> { > >>>> int ret; > >>>> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) > >>>> 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) > >>>> + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); > >>>> + if (!cio2->output) > >>>> return ret; > >>>> > >>>> return 0;
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 4f1ab72debf8..9fa59c1bc97e 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -24,6 +24,28 @@ namespace libcamera { LOG_DEFINE_CATEGORY(IPU3) +struct ImguDevice { + ImguDevice() + : imgu(nullptr), input(nullptr), output(nullptr), + viewfinder(nullptr), stat(nullptr) {} + + ~ImguDevice() + { + delete imgu; + delete input; + delete output; + delete viewfinder; + delete stat; + } + + 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) {} @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData { public: Cio2Device cio2; + ImguDevice *imgu; Stream stream_; }; @@ -71,6 +94,10 @@ public: bool match(DeviceEnumerator *enumerator); private: + static constexpr unsigned int IMGU_PAD_INPUT = 0; + static constexpr unsigned int IMGU_PAD_OUTPUT = 2; + static constexpr unsigned int IMGU_PAD_VF = 3; + static constexpr unsigned int IMGU_PAD_STAT = 4; static constexpr unsigned int IPU3_BUF_NUM = 4; IPU3CameraData *cameraData(const Camera *camera) @@ -79,9 +106,17 @@ private: PipelineHandler::cameraData(camera)); } + int linkImgu(ImguDevice *imgu); + + V4L2Device *openDevice(MediaDevice *media, std::string &name); + V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name); + int initImgu(ImguDevice *imgu); int initCio2(unsigned int index, Cio2Device *cio2); void registerCameras(); + ImguDevice imgu0_; + ImguDevice imgu1_; + std::shared_ptr<MediaDevice> cio2MediaDev_; std::shared_ptr<MediaDevice> imguMediaDev_; }; @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, V4L2DeviceFormat devFormat = {}; int ret; + /* + * TODO: dynamically assign ImgU devices; as of now, with a single + * stream supported, always use 'imgu0'. + */ + data->imgu = &imgu0_; + ret = linkImgu(data->imgu); + if (ret) + return ret; + /* * FIXME: as of now, the format gets applied to the sensor and is * propagated along the pipeline. It should instead be applied on the @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) if (cio2MediaDev_->open()) goto error_release_mdev; + if (imguMediaDev_->open()) + goto error_close_mdev; + if (cio2MediaDev_->disableLinks()) - goto error_close_cio2; + goto error_close_mdev; + + if (initImgu(&imgu0_)) + goto error_close_mdev; + + if (initImgu(&imgu1_)) + goto error_close_mdev; + registerCameras(); cio2MediaDev_->close(); + imguMediaDev_->close(); return true; -error_close_cio2: +error_close_mdev: cio2MediaDev_->close(); + imguMediaDev_->close(); error_release_mdev: cio2MediaDev_->release(); @@ -353,6 +409,153 @@ error_release_mdev: return false; } +/* Link entities in the ImgU unit to prepare for capture operations. */ +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) +{ + MediaLink *link; + int ret; + + unsigned int index = imguDevice == &imgu0_ ? 0 : 1; + std::string imguName = "ipu3-imgu " + std::to_string(index); + std::string inputName = imguName + " input"; + std::string outputName = imguName + " output"; + std::string viewfinderName = imguName + " viewfinder"; + std::string statName = imguName + " 3a stat"; + + ret = imguMediaDev_->open(); + if (ret) + return ret; + + ret = imguMediaDev_->disableLinks(); + if (ret) { + imguMediaDev_->close(); + return ret; + } + + /* Link entities to configure the IMGU unit for capture. */ + link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT); + if (!link) { + LOG(IPU3, Error) + << "Failed to get link '" << inputName << "':0 -> '" + << imguName << "':0"; + ret = -ENODEV; + goto error_close_mediadev; + } + link->setEnabled(true); + + link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0); + if (!link) { + LOG(IPU3, Error) + << "Failed to get link '" << imguName << "':2 -> '" + << outputName << "':0"; + ret = -ENODEV; + goto error_close_mediadev; + } + link->setEnabled(true); + + link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0); + if (!link) { + LOG(IPU3, Error) + << "Failed to get link '" << imguName << "':3 -> '" + << viewfinderName << "':0"; + ret = -ENODEV; + goto error_close_mediadev; + } + link->setEnabled(true); + + link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0); + if (!link) { + LOG(IPU3, Error) + << "Failed to get link '" << imguName << "':4 -> '" + << statName << "':0"; + ret = -ENODEV; + goto error_close_mediadev; + } + link->setEnabled(true); + + imguMediaDev_->close(); + + return 0; + +error_close_mediadev: + imguMediaDev_->close(); + + return ret; + +} + +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media, + std::string &name) +{ + V4L2Device *dev; + + MediaEntity *entity = media->getEntityByName(name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << name << "'"; + return nullptr; + } + + dev = new V4L2Device(entity); + if (dev->open()) + return nullptr; + + return dev; +} + +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media, + std::string &name) +{ + V4L2Subdevice *dev; + + MediaEntity *entity = media->getEntityByName(name); + if (!entity) { + LOG(IPU3, Error) + << "Failed to get entity '" << name << "'"; + return nullptr; + } + + dev = new V4L2Subdevice(entity); + if (dev->open()) + return nullptr; + + return dev; +} + +/* Create video devices and subdevices for the ImgU instance. */ +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu) +{ + unsigned int index = imgu == &imgu0_ ? 0 : 1; + std::string imguName = "ipu3-imgu " + std::to_string(index); + std::string devName; + + imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName); + if (!imgu->imgu) + return -ENODEV; + + devName = imguName + " input"; + imgu->input = openDevice(imguMediaDev_.get(), devName); + if (!imgu->input) + return -ENODEV; + + devName = imguName + " output"; + imgu->output = openDevice(imguMediaDev_.get(), devName); + if (!imgu->output) + return -ENODEV; + + devName = imguName + " viewfinder"; + imgu->viewfinder = openDevice(imguMediaDev_.get(), devName); + if (!imgu->viewfinder) + return -ENODEV; + + devName = imguName + " 3a stat"; + imgu->stat = openDevice(imguMediaDev_.get(), devName); + if (!imgu->stat) + return -ENODEV; + + return 0; +} + int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) { int ret; @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2) 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) + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name); + if (!cio2->output) return ret; return 0;
Create video devices and subdevices associated with an ImgU unit, and link the entities in the media graph to prepare the device for capture operations at stream configuration time. As we support a single stream at the moment, always select imgu0. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++-- 1 file changed, 207 insertions(+), 12 deletions(-)