[libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and link ImgU devices
Jacopo Mondi
jacopo at jmondi.org
Mon Mar 11 09:10:41 CET 2019
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190311/dc9224e2/attachment.sig>
More information about the libcamera-devel
mailing list