[libcamera-devel] [PATCH v3 08/10] libcamera: ipu3: cio2: Make the V4L2 devices private
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Jun 25 22:05:05 CEST 2020
Hi Laurent and Jacopo,
Thanks for your feedback.
On 2020-06-25 04:30:53 +0300, Laurent Pinchart wrote:
> Hi Jacopo and Niklas,
>
> On Wed, Jun 24, 2020 at 12:31:10PM +0200, Jacopo Mondi wrote:
> > On Tue, Jun 23, 2020 at 04:09:28AM +0200, Niklas Söderlund wrote:
> > > In order to make the CIO2 easier to extend with new features make the
> > > V4L2 devices (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>
> >
> > Much better than accessing parts of the CIO2 device from the other
> > components.
> >
> > > ---
> > > * Changes since v2
> > > - Style changes.
> > >
> > > * Changes since v1
> > > - Drop sensor access helpers and replace with a sensor() call to get
> > > hold of the CameraSensor pointer directly.
> > > ---
> > > src/libcamera/pipeline/ipu3/cio2.cpp | 20 +++++++++++++++++++-
> > > src/libcamera/pipeline/ipu3/cio2.h | 17 ++++++++++++++---
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++-----------
> > > 3 files changed, 41 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > index d6bab896706dd60e..3d7348782b0fa6cb 100644
> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > @@ -38,7 +38,7 @@ static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = {
> > > } /* namespace */
> > >
> > > CIO2Device::CIO2Device()
> > > - : output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> > > + : sensor_(nullptr), csi2_(nullptr), output_(nullptr)
> > > {
> > > }
> > >
> > > @@ -126,6 +126,8 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > > if (ret)
> > > return ret;
> > >
> > > + output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady);
> >
> > There's a bit of signal ping-pong here...
> > Could this be avoided by providing a registerBufferReady() method that
> > the pipielinehandler can call and provide a slot to connect to the
> > video device signal ? I can't actually tell how bad this is tbh
>
> How about
>
> - Signal<FrameBuffer *> bufferReady;
> + Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
>
> ? That would avoid proxying the signal while still not exposing the
> V4L2VideoDevice.
That would work in this pathc but 10/10 I inject code into
CIO2Device::cio2BufferReady() which require the signal to be proxied. So
taking this in here I fear would only be busy work :-)
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -226,6 +228,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. */
> > > @@ -266,4 +274,14 @@ int CIO2Device::stop()
> > > return output_->streamOff();
> > > }
> > >
> > > +int CIO2Device::queueBuffer(FrameBuffer *buffer)
> > > +{
> > > + return output_->queueBuffer(buffer);
> > > +}
> > > +
> > > +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 6276573f2b585b25..cc898f13fd73f865 100644
> > > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > > @@ -11,6 +11,8 @@
> > > #include <queue>
> > > #include <vector>
> > >
> > > +#include <libcamera/signal.h>
> > > +
> > > namespace libcamera {
> > >
> > > class CameraSensor;
> > > @@ -36,6 +38,8 @@ public:
> > > StreamConfiguration generateConfiguration(const Size &desiredSize) const;
> > >
> > > int allocateBuffers();
> > > + int exportBuffers(unsigned int count,
> > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > > void freeBuffers();
> > >
> > > FrameBuffer *getBuffer();
> > > @@ -44,11 +48,18 @@ public:
> > > int start();
> > > int stop();
> > >
> > > - V4L2VideoDevice *output_;
> > > - V4L2Subdevice *csi2_;
> > > - CameraSensor *sensor_;
> > > + CameraSensor *sensor() { return sensor_; }
> > > +
> > > + int queueBuffer(FrameBuffer *buffer);
> > > + Signal<FrameBuffer *> bufferReady;
> > >
> > > private:
> > > + void cio2BufferReady(FrameBuffer *buffer);
> > > +
> > > + CameraSensor *sensor_;
> > > + V4L2Subdevice *csi2_;
> > > + V4L2VideoDevice *output_;
> > > +
> > > 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 c0e727e592f46883..2d1ec707ea4b08fe 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -431,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > >
> > > stream = &data->rawStream_;
> > >
> > > - cfg.size = data->cio2_.sensor_->resolution();
> > > + cfg.size = data->cio2_.sensor()->resolution();
> > >
> > > cfg = data->cio2_.generateConfiguration(cfg.size);
> > > break;
> > > @@ -460,7 +460,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_.sensor()->resolution();
> >
> > This could actually be Cio2Device::resolution().
> > Up to you.
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > unsigned int width = std::min(1280U, res.width);
> > > unsigned int height = std::min(720U, res.height);
> > > cfg.size = { width & ~7, height & ~3 };
> > > @@ -640,14 +640,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);
> > >
> > > - return video->exportBuffers(count, buffers);
> > > + return ipu3stream->device_->dev->exportBuffers(count, buffers);
> > > }
> > >
> > > /**
> > > @@ -757,7 +754,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);
> > > @@ -870,7 +867,7 @@ int PipelineHandlerIPU3::registerCameras()
> > > continue;
> > >
> > > /* Initialize the camera properties. */
> > > - data->properties_ = cio2->sensor_->properties();
> > > + data->properties_ = cio2->sensor()->properties();
> > >
> > > /**
> > > * \todo Dynamically assign ImgU and output devices to each
> > > @@ -894,7 +891,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);
> > > @@ -904,7 +901,7 @@ int PipelineHandlerIPU3::registerCameras()
> > > &IPU3CameraData::imguOutputBufferReady);
> > >
> > > /* Create and register the Camera instance. */
> > > - std::string cameraName = cio2->sensor_->entity()->name();
> > > + std::string cameraName = cio2->sensor()->entity()->name();
> > > std::shared_ptr<Camera> camera = Camera::create(this,
> > > cameraName,
> > > streams);
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list