[libcamera-devel] [PATCH 08/10] libcamera: ipu3: cio2: Make the V4L2 devices private
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 4 05:26:01 CEST 2020
Hello,
On Tue, Jun 02, 2020 at 02:24:39PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 02, 2020 at 03:39:07AM +0200, Niklas Söderlund wrote:
> > In order to make the CIO2 easier to extend with new features make the
> > V4L2 deices (sensor, CIO2 and video device) private members. This
> > requires a few helper functions to be added to allow for the IPU3 driver
> > to still be able to interact with all parts of the CIO2. These helper
> > functions will later be extended to add new features to the IPU3
> > pipeline.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/ipu3/cio2.cpp | 18 ++++++++++++++++++
> > src/libcamera/pipeline/ipu3/cio2.h | 19 +++++++++++++++----
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++++++---------
> > 3 files changed, 40 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 2263d6530ec6b672..63a46959f3cac06a 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -98,6 +98,8 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > if (ret)
> > return ret;
> >
> > + output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady);
> > +
> > return 0;
> > }
> >
> > @@ -218,6 +220,12 @@ int CIO2Device::allocateBuffers()
> > return ret;
> > }
> >
> > +int CIO2Device::exportBuffers(unsigned int count,
> > + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > + return output_->exportBuffers(count, buffers);
> > +}
> > +
> > void CIO2Device::freeBuffers()
> > {
> > /* The default std::queue constructor is explicit with gcc 5 and 6. */
> > @@ -258,6 +266,11 @@ int CIO2Device::stop()
> > return output_->streamOff();
> > }
> >
> > +int CIO2Device::queueBuffer(FrameBuffer *buffer)
> > +{
> > + return output_->queueBuffer(buffer);
> > +}
> > +
> > V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> > {
> > switch (code) {
> > @@ -274,4 +287,9 @@ V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> > }
> > }
> >
> > +void CIO2Device::cio2BufferReady(FrameBuffer *buffer)
> > +{
> > + bufferReady.emit(buffer);
> > +}
> > +
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index 2e268a7154b2d241..465c0778f27e4066 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -27,7 +27,7 @@ public:
> > static constexpr unsigned int CIO2_BUFFER_COUNT = 4;
> >
> > CIO2Device()
> > - : output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> > + : sensor_(nullptr), csi2_(nullptr), output_(nullptr)
>
> I wonder if sensor_ should be part of CIO2 at all
> I see below a CIO2Device::resolution() which I'm not super happy
> about...
>
> Could the sensor live in the CameraData and CameraData be passed to
> the CIO2Device, if it needs it ?
I was going to write the same :-)
> > {
> > }
> >
> > @@ -45,6 +45,8 @@ public:
> > const Size desiredSize = {}) const;
> >
> > int allocateBuffers();
> > + int exportBuffers(unsigned int count,
> > + std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > void freeBuffers();
> >
> > FrameBuffer *getBuffer();
> > @@ -53,13 +55,22 @@ public:
> > int start();
> > int stop();
> >
> > + const std::string &name() const { return sensor_->entity()->name(); }
> > + Size resolution() const { return sensor_->resolution(); }
> > + ControlList properties() const { return sensor_->properties(); }
>
> I really don't like these three :(
Agreed.
> > +
> > + int queueBuffer(FrameBuffer *buffer);
> > + Signal<FrameBuffer *> bufferReady;
>
> You could inline their single-line implementations here
>
> > +
> > +private:
> > static V4L2PixelFormat mediaBusToFormat(unsigned int code);
> >
> > - V4L2VideoDevice *output_;
> > - V4L2Subdevice *csi2_;
> > + void cio2BufferReady(FrameBuffer *buffer);
> > +
> > CameraSensor *sensor_;
> > + V4L2Subdevice *csi2_;
> > + V4L2VideoDevice *output_;
> >
> > -private:
> > std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> > std::queue<FrameBuffer *> availableBuffers_;
> > };
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 56cc3ca10414f0d2..2d636f0944587a17 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -454,7 +454,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > * available sensor resolution and to the IPU3
> > * alignment constraints.
> > */
> > - const Size &res = data->cio2_.sensor_->resolution();
> > + const Size &res = data->cio2_.resolution();
>
> This would really be better as data->sensor_.resolution() assuming
> that's possible.
>
> > unsigned int width = std::min(1280U, res.width);
> > unsigned int height = std::min(720U, res.height);
> > cfg.size = { width & ~7, height & ~3 };
> > @@ -634,13 +634,11 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > IPU3CameraData *data = cameraData(camera);
> > IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> > unsigned int count = stream->configuration().bufferCount;
> > - V4L2VideoDevice *video;
> >
> > if (ipu3stream->raw_)
> > - video = data->cio2_.output_;
> > - else
> > - video = ipu3stream->device_->dev;
> > + return data->cio2_.exportBuffers(count, buffers);
> >
> > + V4L2VideoDevice *video = ipu3stream->device_->dev;
> > return video->exportBuffers(count, buffers);
> > }
> >
> > @@ -749,7 +747,7 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> > return -EINVAL;
> >
> > buffer->setRequest(request);
> > - data->cio2_.output_->queueBuffer(buffer);
> > + data->cio2_.queueBuffer(buffer);
> >
> > for (auto it : request->buffers()) {
> > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> > @@ -862,7 +860,7 @@ int PipelineHandlerIPU3::registerCameras()
> > continue;
> >
> > /* Initialize the camera properties. */
> > - data->properties_ = cio2->sensor_->properties();
> > + data->properties_ = cio2->properties();
> >
> > /**
> > * \todo Dynamically assign ImgU and output devices to each
> > @@ -886,7 +884,7 @@ int PipelineHandlerIPU3::registerCameras()
> > * associated ImgU input where they get processed and
> > * returned through the ImgU main and secondary outputs.
> > */
> > - data->cio2_.output_->bufferReady.connect(data.get(),
> > + data->cio2_.bufferReady.connect(data.get(),
> > &IPU3CameraData::cio2BufferReady);
> > data->imgu_->input_->bufferReady.connect(data.get(),
> > &IPU3CameraData::imguInputBufferReady);
> > @@ -896,7 +894,7 @@ int PipelineHandlerIPU3::registerCameras()
> > &IPU3CameraData::imguOutputBufferReady);
> >
> > /* Create and register the Camera instance. */
> > - std::string cameraName = cio2->sensor_->entity()->name();
> > + std::string cameraName = cio2->name();
>
> Same here and above.
>
> > std::shared_ptr<Camera> camera = Camera::create(this,
> > cameraName,
> > streams);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list