[libcamera-devel] [PATCH v5 11/19] libcamera: ipu3: Create ImgUDevice class
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 2 12:53:54 CEST 2019
Hi Laurent,
On Tue, Apr 02, 2019 at 01:43:17PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Apr 02, 2019 at 12:37:32PM +0200, Jacopo Mondi wrote:
> > On Tue, Apr 02, 2019 at 12:26:13PM +0300, Laurent Pinchart wrote:
> > > 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 :)
>
> Correct. Your spell checker likely only reports the use of Incorrect
> English (https://lwn.net/Articles/636286/).
>
That's certainly the version of English I know best.
> > >> + *
> > >> + * 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.
>
> This is internal code so it shouldn't be too dangerous (would be another
> story if the object was exposed to applications, or even just within
> libcamera). I would have a slight preference for correctness myself, but
> I agree that not having to destroy the objects is a good argument
> against this change. Pros and cons, I'll let you decide :-)
>
I would keep them statically allocated if that's fine with you, as you
said, this is internal code after all...
> > >> +{
> > >> + 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?
>
> Possibly not. If you don't check this, please add a comment that
> explains why the check is not needed.
>
Sure thing.
> > > With these small issues fixed,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
I'll take your tag in with the above clarified.
Thanks
j
> > >
> > >> + 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/1b8724fc/attachment-0001.sig>
More information about the libcamera-devel
mailing list