[libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and link ImgU devices
Jacopo Mondi
jacopo at jmondi.org
Sat Mar 9 21:50:18 CET 2019
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 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.
> > + 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
-------------- 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/20190309/a7a1ee9c/attachment-0001.sig>
More information about the libcamera-devel
mailing list