[libcamera-devel] [PATCH 13/13] libcamera: ipu3: imgu: Remove ImgUOutput
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jun 28 01:47:47 CEST 2020
Hi Laurent,
Thanks for your feedback.
On 2020-06-27 19:52:04 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Sat, Jun 27, 2020 at 05:00:43AM +0200, Niklas Söderlund wrote:
> > The struct ImgUOutput now only contains one member that are in use, the
> > video device. Remove the struct and use the video device directly
> > instead.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/ipu3/imgu.cpp | 55 ++++++++++++----------------
> > src/libcamera/pipeline/ipu3/imgu.h | 25 ++++---------
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++---
> > 3 files changed, 37 insertions(+), 55 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index d54e844d9bfc5147..a601ca3fa2b1803f 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -55,31 +55,22 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
> > if (ret)
> > return ret;
> >
> > - output_.dev = V4L2VideoDevice::fromEntityName(media, name_ + " output");
> > - ret = output_.dev->open();
> > + output_ = V4L2VideoDevice::fromEntityName(media, name_ + " output");
> > + ret = output_->open();
> > if (ret)
> > return ret;
> >
> > - output_.pad = PAD_OUTPUT;
> > - output_.name = "output";
> > -
> > - viewfinder_.dev = V4L2VideoDevice::fromEntityName(media,
> > + viewfinder_ = V4L2VideoDevice::fromEntityName(media,
> > name_ + " viewfinder");
>
> Wrong indentation.
>
> > - ret = viewfinder_.dev->open();
> > + ret = viewfinder_->open();
> > if (ret)
> > return ret;
> >
> > - viewfinder_.pad = PAD_VF;
> > - viewfinder_.name = "viewfinder";
> > -
> > - stat_.dev = V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat");
> > - ret = stat_.dev->open();
> > + stat_ = V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat");
> > + ret = stat_->open();
> > if (ret)
> > return ret;
> >
> > - stat_.pad = PAD_STAT;
> > - stat_.name = "stat";
> > -
> > return 0;
> > }
> >
> > @@ -142,19 +133,19 @@ int ImgUDevice::configureInput(const Size &size,
> > int ImgUDevice::configureOutput(const StreamConfiguration &cfg,
> > V4L2DeviceFormat *outputFormat)
> > {
> > - return configureVideoDevice(output_.dev, PAD_OUTPUT, cfg, outputFormat);
> > + return configureVideoDevice(output_, PAD_OUTPUT, cfg, outputFormat);
> > }
> >
> > int ImgUDevice::configureViewfinder(const StreamConfiguration &cfg,
> > V4L2DeviceFormat *outputFormat)
> > {
> > - return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg, outputFormat);
> > + return configureVideoDevice(viewfinder_, PAD_VF, cfg, outputFormat);
> > }
> >
> > int ImgUDevice::configureStat(const StreamConfiguration &cfg,
> > V4L2DeviceFormat *outputFormat)
> > {
> > - return configureVideoDevice(stat_.dev, PAD_STAT, cfg, outputFormat);
> > + return configureVideoDevice(stat_, PAD_STAT, cfg, outputFormat);
> > }
> >
> > int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > @@ -170,7 +161,7 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > return ret;
> >
> > /* No need to apply format to the stat node. */
> > - if (dev == stat_.dev)
> > + if (dev == stat_)
> > return 0;
> >
> > *outputFormat = {};
> > @@ -182,7 +173,7 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > if (ret)
> > return ret;
> >
> > - const char *name = dev == output_.dev ? "output" : "viewfinder";
> > + const char *name = dev == output_ ? "output" : "viewfinder";
> > LOG(IPU3, Debug) << "ImgU " << name << " format = "
> > << outputFormat->toString();
> >
> > @@ -208,19 +199,19 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)
> > *
> > * \todo To be revised when we'll actually use the stat node.
> > */
> > - ret = stat_.dev->importBuffers(bufferCount);
> > + ret = stat_->importBuffers(bufferCount);
> > if (ret < 0) {
> > LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers";
> > goto error;
> > }
> >
> > - ret = output_.dev->importBuffers(bufferCount);
> > + ret = output_->importBuffers(bufferCount);
> > if (ret < 0) {
> > LOG(IPU3, Error) << "Failed to import ImgU output buffers";
> > goto error;
> > }
> >
> > - ret = viewfinder_.dev->importBuffers(bufferCount);
> > + ret = viewfinder_->importBuffers(bufferCount);
> > if (ret < 0) {
> > LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers";
> > goto error;
> > @@ -241,15 +232,15 @@ void ImgUDevice::freeBuffers()
> > {
> > int ret;
> >
> > - ret = output_.dev->releaseBuffers();
> > + ret = output_->releaseBuffers();
> > if (ret)
> > LOG(IPU3, Error) << "Failed to release ImgU output buffers";
> >
> > - ret = stat_.dev->releaseBuffers();
> > + ret = stat_->releaseBuffers();
> > if (ret)
> > LOG(IPU3, Error) << "Failed to release ImgU stat buffers";
> >
> > - ret = viewfinder_.dev->releaseBuffers();
> > + ret = viewfinder_->releaseBuffers();
> > if (ret)
> > LOG(IPU3, Error) << "Failed to release ImgU viewfinder buffers";
> >
> > @@ -263,19 +254,19 @@ int ImgUDevice::start()
> > int ret;
> >
> > /* Start the ImgU video devices. */
> > - ret = output_.dev->streamOn();
> > + ret = output_->streamOn();
> > if (ret) {
> > LOG(IPU3, Error) << "Failed to start ImgU output";
> > return ret;
> > }
> >
> > - ret = viewfinder_.dev->streamOn();
> > + ret = viewfinder_->streamOn();
> > if (ret) {
> > LOG(IPU3, Error) << "Failed to start ImgU viewfinder";
> > return ret;
> > }
> >
> > - ret = stat_.dev->streamOn();
> > + ret = stat_->streamOn();
> > if (ret) {
> > LOG(IPU3, Error) << "Failed to start ImgU stat";
> > return ret;
> > @@ -294,9 +285,9 @@ int ImgUDevice::stop()
> > {
> > int ret;
> >
> > - ret = output_.dev->streamOff();
> > - ret |= viewfinder_.dev->streamOff();
> > - ret |= stat_.dev->streamOff();
> > + ret = output_->streamOff();
> > + ret |= viewfinder_->streamOff();
> > + ret |= stat_->streamOff();
> > ret |= input_->streamOff();
> >
> > return ret;
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> > index f8fd54089753b40e..2ac77ce5719f88f1 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > @@ -24,28 +24,19 @@ struct StreamConfiguration;
> > class ImgUDevice
> > {
> > public:
> > - /* ImgU output descriptor: group data specific to an ImgU output. */
> > - struct ImgUOutput {
> > - V4L2VideoDevice *dev;
> > - unsigned int pad;
> > - std::string name;
> > - };
> > -
> > ImgUDevice()
> > - : imgu_(nullptr), input_(nullptr)
> > + : imgu_(nullptr), input_(nullptr), output_(nullptr),
> > + viewfinder_(nullptr), stat_(nullptr)
> > {
> > - output_.dev = nullptr;
> > - viewfinder_.dev = nullptr;
> > - stat_.dev = nullptr;
> > }
> >
> > ~ImgUDevice()
> > {
> > delete imgu_;
> > delete input_;
> > - delete output_.dev;
> > - delete viewfinder_.dev;
> > - delete stat_.dev;
> > + delete output_;
> > + delete viewfinder_;
> > + delete stat_;
> > }
> >
> > int init(MediaDevice *media, unsigned int index);
> > @@ -68,9 +59,9 @@ public:
> >
> > V4L2Subdevice *imgu_;
> > V4L2VideoDevice *input_;
> > - ImgUOutput output_;
> > - ImgUOutput viewfinder_;
> > - ImgUOutput stat_;
> > + V4L2VideoDevice *output_;
> > + V4L2VideoDevice *viewfinder_;
> > + V4L2VideoDevice *stat_;
>
> You could turn these into unique_ptr to simplify the constructor and
> destructor.
Good idea, I will add a patch for v2 to do just that.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > /* \todo Add param video device for 3A tuning */
> >
> > private:
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 6ae4728b470f9487..c25bcaaeb5fb813f 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -561,9 +561,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > unsigned int count = stream->configuration().bufferCount;
> >
> > if (stream == &data->outStream_)
> > - return data->imgu_.output_.dev->exportBuffers(count, buffers);
> > + return data->imgu_.output_->exportBuffers(count, buffers);
> > else if (stream == &data->vfStream_)
> > - return data->imgu_.viewfinder_.dev->exportBuffers(count, buffers);
> > + return data->imgu_.viewfinder_->exportBuffers(count, buffers);
> > else if (stream == &data->rawStream_)
> > return data->cio2_.exportBuffers(count, buffers);
> >
> > @@ -678,9 +678,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> > int ret;
> >
> > if (stream == &data->outStream_)
> > - ret = data->imgu_.output_.dev->queueBuffer(buffer);
> > + ret = data->imgu_.output_->queueBuffer(buffer);
> > else if (stream == &data->vfStream_)
> > - ret = data->imgu_.viewfinder_.dev->queueBuffer(buffer);
> > + ret = data->imgu_.viewfinder_->queueBuffer(buffer);
> > else
> > continue;
> >
> > @@ -802,9 +802,9 @@ int PipelineHandlerIPU3::registerCameras()
> > &IPU3CameraData::cio2BufferReady);
> > data->imgu_.input_->bufferReady.connect(&data->cio2_,
> > &CIO2Device::tryReturnBuffer);
> > - data->imgu_.output_.dev->bufferReady.connect(data.get(),
> > + data->imgu_.output_->bufferReady.connect(data.get(),
> > &IPU3CameraData::imguOutputBufferReady);
> > - data->imgu_.viewfinder_.dev->bufferReady.connect(data.get(),
> > + data->imgu_.viewfinder_->bufferReady.connect(data.get(),
> > &IPU3CameraData::imguOutputBufferReady);
> >
> > /* Create and register the Camera instance. */
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list