[libcamera-devel] [PATCH v5 11/19] libcamera: ipu3: Create ImgUDevice class
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 2 12:37:32 CEST 2019
Hi Laurent,
On Tue, Apr 02, 2019 at 12:26:13PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Mar 26, 2019 at 09:38:54AM +0100, Jacopo Mondi wrote:
> > Group ImgU components in a class associated with a camera at camera
> > registration time and provide an intialization method to create and open
> > all video devices and subdevices of the ImgU.
> >
> > Statically assign imgu0 to the first camera and imgu1 to the second one
> > and limit support to two cameras. This will have to be revised in the future.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 154 +++++++++++++++++++++++++--
> > 1 file changed, 145 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 21205b39afee..0a059b75cadf 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -44,6 +44,43 @@ static int mediaBusToCIO2Format(unsigned int code)
> > }
> > }
> >
> > +class ImgUDevice
> > +{
> > +public:
> > + static constexpr unsigned int PAD_INPUT = 0;
> > + static constexpr unsigned int PAD_OUTPUT = 2;
> > + static constexpr unsigned int PAD_VF = 3;
> > + static constexpr unsigned int PAD_STAT = 4;
> > +
> > + ImgUDevice()
> > + : imgu_(nullptr), input_(nullptr), output_(nullptr),
> > + viewfinder_(nullptr), stat_(nullptr)
> > + {
> > + }
> > +
> > + ~ImgUDevice()
> > + {
> > + delete imgu_;
> > + delete input_;
> > + delete output_;
> > + delete viewfinder_;
> > + delete stat_;
> > + }
> > +
> > + int init(MediaDevice *media, unsigned int index);
> > +
> > + unsigned int index_;
> > + std::string name_;
> > + MediaDevice *media_;
> > +
> > + V4L2Subdevice *imgu_;
> > + V4L2Device *input_;
> > + V4L2Device *output_;
> > + V4L2Device *viewfinder_;
> > + V4L2Device *stat_;
> > + /* \todo Add param video device for 3A tuning */
> > +};
> > +
> > class CIO2Device
> > {
> > public:
> > @@ -106,6 +143,7 @@ private:
> > void bufferReady(Buffer *buffer);
> >
> > CIO2Device cio2_;
> > + ImgUDevice *imgu_;
> >
> > Stream stream_;
> > };
> > @@ -116,8 +154,10 @@ private:
> > PipelineHandler::cameraData(camera));
> > }
> >
> > - void registerCameras();
> > + int registerCameras();
> >
> > + ImgUDevice imgu0_;
> > + ImgUDevice imgu1_;
> > std::shared_ptr<MediaDevice> cio2MediaDev_;
> > std::shared_ptr<MediaDevice> imguMediaDev_;
> > };
> > @@ -303,6 +343,8 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> >
> > bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > {
> > + int ret;
> > +
> > DeviceMatch cio2_dm("ipu3-cio2");
> > cio2_dm.add("ipu3-csi2 0");
> > cio2_dm.add("ipu3-cio2 0");
> > @@ -358,36 +400,75 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > return false;
> > }
> >
> > - registerCameras();
> > + if (imguMediaDev_->open()) {
> > + cio2MediaDev_->close();
> > + return false;
> > + }
> > +
> > + if (imguMediaDev_->disableLinks())
> > + goto error;
> > +
> > + ret = registerCameras();
> > + if (ret)
> > + goto error;
> >
> > cio2MediaDev_->close();
> > + imguMediaDev_->close();
> >
> > return true;
> > +
> > +error:
> > + cio2MediaDev_->close();
> > + imguMediaDev_->close();
> > +
> > + return false;
> > }
> >
> > -/*
> > - * Cameras are created associating an image sensor (represented by a
> > - * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
> > - * CIO2 CSI-2 receivers.
> > +/**
> > + * \brief Initialize ImgU devices and CIO2 ones associated with cameras
>
> s/ImgU devices and CIO2 ones/the ImgU and CIO2 devices/
>
> I'm sure Kieran would also tell you to s/Initialize/Initialise/ :-)
>
Is this a british vs american english thing ? My spell checker does
not report 'initialize' as wrong :)
> > + *
> > + * Initialize the two ImgU instances and create cameras with an associated
> > + * CIO2 device instance.
> > + *
> > + * \return 0 on success or a negative error code for error or if no camera
> > + * has been created
> > + * \retval -ENODEV no camera has been created
> > */
> > -void PipelineHandlerIPU3::registerCameras()
> > +int PipelineHandlerIPU3::registerCameras()
> > {
> > + int ret;
> > +
> > + ret = imgu0_.init(imguMediaDev_.get(), 0);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imgu1_.init(imguMediaDev_.get(), 1);
> > + if (ret)
> > + return ret;
> > +
> > /*
> > * For each CSI-2 receiver on the IPU3, create a Camera if an
> > * image sensor is connected to it and it can produce images in
> > * a compatible format.
> > */
> > unsigned int numCameras = 0;
> > - for (unsigned int id = 0; id < 4; ++id) {
> > + for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
> > std::unique_ptr<IPU3CameraData> data =
> > utils::make_unique<IPU3CameraData>(this);
> > std::set<Stream *> streams{ &data->stream_ };
> > CIO2Device *cio2 = &data->cio2_;
> >
> > - int ret = cio2->init(cio2MediaDev_.get(), id);
> > + ret = cio2->init(cio2MediaDev_.get(), id);
> > if (ret)
> > continue;
> >
> > + /**
> > + * \todo Dynamically assign ImgU devices; as of now, limit
> > + * support to two cameras only, and assign imgu0 to the first
> > + * one and imgu1 to the second.
> > + */
> > + data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> > +
> > std::string cameraName = cio2->name() + " "
> > + std::to_string(id);
> > std::shared_ptr<Camera> camera = Camera::create(this,
> > @@ -406,6 +487,8 @@ void PipelineHandlerIPU3::registerCameras()
> >
> > numCameras++;
> > }
> > +
> > + return numCameras ? 0 : -ENODEV;
> > }
> >
> > void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)
> > @@ -416,6 +499,59 @@ void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)
> > pipe_->completeRequest(camera_, request);
> > }
> >
> > +/* -----------------------------------------------------------------------------
> > + * ImgU Device
> > + */
> > +
> > +/**
> > + * \brief Initialize components of the ImgU instance
> > + * \param[in] mediaDevice The ImgU instance media device
> > + * \param[in] index The ImgU instance index
> > + *
> > + * Create and open the V4L2 devices and subdevices of the ImgU instance
> > + * with \a index.
> > + *
> > + * In case of errors the created V4L2Device and V4L2Subdevice instances
> > + * are destroyed at pipeline handler delete time.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int ImgUDevice::init(MediaDevice *media, unsigned int index)
>
> It could make sense to pass the media device pointer and index to the
> constructor, in order to avoid leaving the media_ and index_ members
> unitialized. The initialization of course belongs here. This would imply
> creating the ImgUDevice instances dynamically. What do you think ? This
> comment also applies to the previous patch.
The ImgU units initialization happens as first thing at
registerCameras() time, if any of two fails, the match() function
fails (registerCameras() now returns an int).
I'm not sure where it would be risky to have those two member
uninitialized, but I understand the concern.
I usually try to minimize run-time allocations and I like the fact the
ImgU and CIO2 devices lifecycle is automatically tight to the
cameraData's one, but it shouldn't be hard to handle them in the
destructor. If the change is worth in your opinion, I'll address it.
>
> > +{
> > + int ret;
> > +
> > + index_ = index;
> > + name_ = "ipu3-imgu " + std::to_string(index_);
> > + media_ = media;
> > +
> > + imgu_ = V4L2Subdevice::fromEntityName(media, name_);
>
> Shouldn't test if this (and the other V4L2Device instances below) is
> null before trying to open it on the next line ?
The match function has validated that the entity is present in the
media device, the only reason this could fail is if we run out of
memory and the 'new V4L2Subdevice()' call fails. Is this worth
checking?
Thanks
j
>
> With these small issues fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + ret = imgu_->open();
> > + if (ret)
> > + return ret;
> > +
> > + input_ = V4L2Device::fromEntityName(media, name_ + " input");
> > + ret = input_->open();
> > + if (ret)
> > + return ret;
> > +
> > + output_ = V4L2Device::fromEntityName(media, name_ + " output");
> > + ret = output_->open();
> > + if (ret)
> > + return ret;
> > +
> > + viewfinder_ = V4L2Device::fromEntityName(media, name_ + " viewfinder");
> > + ret = viewfinder_->open();
> > + if (ret)
> > + return ret;
> > +
> > + stat_ = V4L2Device::fromEntityName(media, name_ + " 3a stat");
> > + ret = stat_->open();
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > /*------------------------------------------------------------------------------
> > * CIO2 Device
> > */
>
> --
> 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/20190402/6bb7029b/attachment.sig>
More information about the libcamera-devel
mailing list